Auto pairs delete

[dead10ck]

I began trying to implement deleting pairs on backspace, and I ran into an issue that I think is best demonstrated by showing what I've tried first. This PR is mainly just context for a discussion at this stage; it made me think a more invasive change may be necessary to support it.

Looking over the current code for delete_char_backward, I can see that there is code there for handling dedenting as well. In order to preserve this behavior, we need to basically check for both conditions on a backspace: if the line is all whitespace to dedent, and if not, then check if both sides of the cursor are a pair, and finally proceeding to a regular single character deletion.

Now, the current code for auto pairs works by putting together an entire transaction, assuming its logic will either create the entire change or none of it. However, with multiple ranges in the selection, one cursor could end up being in position for an auto pair deletion while another is on a line that only contains whitespace, and thus should dedent. In order to support this, we need to produce changes for individual ranges, not on the whole selection.

So the PR here is what I tried to get it working. And it does work, but only for one-width cursors. If it's in a selection, the head ends up moving back two characters, when it should only move back once, e.g. [word(]) -> [wor]d. I think this is the same reason that the current auto pairs code works with entire Transactions, since by doing this, we can calculate the correct resulting selection as well as the textual changes to the document.

So we have a conflict here: auto pairs needs to change selections as well as text, but we also cannot control the entire transaction, because auto pairs is not responsible for all of the changes that could happen.

So I had an idea that maybe Change could become a 4-tuple, with the last being an Option<Range> that explicitly sets what the new range should be after the textual change. Then we could have Transaction::change build up a selection as it iterates through the Changes. Then we could build the transactions the same way, but we'd get selection manipulation too.

So the hook functions would return Option<Change>, and operate only on a single range of text. I believe this would also, by extension, take care of the old todo that the auto pairs insert hook should just fall back on the default insert hook when it won't insert a pair.

What do you think?

[archseer]

What about relying on OT? Each hook can generate a transaction, then we fold(map+compose) them to generate a transaction with all the changes. Map is currently unfinished but could be based on #659

[dead10ck]

Hmm, just looking over the code for compose, it looks like at least the current implementation will make a whole new change set for any two inputs, which means that using it n times to combine all the ranges would be exponential on the number of selections / changes. This would quickly become a big problem on large files with lots of selections. I imagine for, example, a file with 10k lines and a cursor on every line would bring helix to its knees.

[archseer]

Oh I mean, rather than doing this per range, each hook should return it's own transaction, then we map+compose all of them together and apply. So it would be n operations, equal to the number of hooks.

[archseer]

I guess the problem is that won't work with with explicit position setting, hmm...

[dead10ck]

Yes, it would be n calls to compose, but each call builds a new Transaction by copying elements from both, so e.g. if you have 10 selections, you get 10 Transactions and 10 calls to compose, and so on call 1, you have t¹ + t², then on call 2, you have t¹ + t² + t³, then t¹ + t² + t³ + t⁴, etc, etc. If my math is not mistaken, it becomes ½(n² – n)

[archseer]

It's not quite like that, it calculates a new transaction that has the same result as applying a, then applying b. So if it's an insert ab, followed by a delete 1, it calculates a new transaction that's only insert a.

Each transaction also affects the whole selection, it's one tx per selection, not selection range.

[dead10ck]

Oh I'm sorry, I'm just understanding what you meant by this.

Oh I mean, rather than doing this per range, each hook should return it's own transaction, then we map+compose all of them together and apply. So it would be n operations, equal to the number of hooks.

So, sorry, let me see if I'm understanding what the algorithm becomes. If we are generating a whole transaction to do a backspace, then if I'm understanding the algorithm right, then the idea is that we'd calculate all 3 possible different transformations (dedent, delete pair, or delete single char) over the whole selection text as if that were the only transformation being applied, and then merging the change sets together. So e.g. if we have

••••|
bar = "|"
foo|

We'd get one transformation that only dedents that first line, one that only deletes the pair on the second line, and one that only deletes a single character on the third, and then mash these together?

In theory I think this sounds like it might work, but thinking through how we calculate these transactions, in the first two transformations, we could make it so only applicable changes get applied, i.e. in the first, we only generate a transaction that dedents, and nothing else, and in the second, we only generate a transaction that deletes pairs. But in the third, there are no conditions for when a delete gets applied—it will just delete one character at every range. So we'd end up with the following transactions:

• Delete 4 on line 1
• Delete 2 on line 2
• Three Delete 1's, one character from each line

How do we know we shouldn't apply the first two deletes from the third one? By that point, all we have is a bunch of deletes, and we can't pick them apart.

I thought maybe it could just be two transactions generated: one where it either dedents or deletes a single char, and one where it either deletes a pair or just a single char; but in this case, you end up with twice as many deletes as you wanted. And then on top of this, yeah, we'd have to figure out how to compose the selections as well as the text transformations.

I might be missing understanding of part of how this would work, but I'm not seeing how we can do this without breaking down the granularity of the transformations from whole transactions to ranges.

And besides all this, we'd still be doing 2-3 passes over the text in the selection instead of 1.

[slinlee]

I have a naive question, but is this approach making it too complicated? My situation is like #1673 where I would want the auto-pair ending to be deleted only if that was the previous thing I just did.

If I'm opening a random file and deleting the second character here : (() I wouldn't want it to delete the closing )

[dead10ck]

I get the desire, but I do think it might make things a bit too complicated in my opinion. We'd have to do special tracking of recent edits to become aware if the insertion of a pair like () was the result of an auto pair or e.g. pasting foo() from a function call or something.

Also, if you just want to delete a single character, you can always just move the cursor over the ( and hit d; this is a completely separate code path than typing a backspace in insert mode, and does not interact with the auto pair mechanism.

[dead10ck]

I'm going to close this, as it was really just for the discussion, so there's no reason to keep it open. When I get to this, I'm going to try the implementation I suggested in the description.