Fix generate_transaction_from_snippet to be less sensitive to selection

[andriigrynenko]

This is a minor follow up to #5864. There's a lot more details and context in Urgau#7.

generate_transaction_from_snippet was previously re-using the original selection and it was making the code very sensitive to the position of that selection relative to the text being replaced. This change updates the logic to create a new selection instead.

[pascalkuthe]

This is theoretically fixes the problem but I need to restructure the way we handle tabstops anyway a bit to fix some of the crashes adressed by #5728. Right now we map the selection trough the transaction and then calculate the tabstops relative to that. That worked well initially but now that we re-render the snapshot for each selection anyway just saving the absolute position of the tabstops is easier and less brittle. I will implement these changes in #5728 because they closely interact with the fixes there (and likely even need small changes to #5728 to correctly account for)

[andriigrynenko]

Right now we map the selection trough the transaction and then calculate the tabstops relative to that. That worked well initially but now that we re-render the snapshot for each selection anyway just saving the absolute position of the tabstops is easier and less brittle

I think I actually did both of these things intentionally in the same commit that added re-rendering the snippet for every selection - 1866b43 (i.e. initially there was no code that's mapping selection through transaction). My idea was the following:

1. Having snippet::render operate on relative positions makes it completely unaware of the rest of the document code, making the separation cleaner - e.g. it easier to test.
2. Mapping transaction through the selection was done to-reuse existing logic inside Transaction that computes the new position of a cursor (or selection) after the transaction.

This change simply continues to follow this logic and makes (2) actually use the cursor at the end of the newly inserted snippet rather than what happens to match that position most of the time :)

My understanding is that change_by_selection creates a Transaction that operates on initial ranges. That means that render_snippet can not calculate the final offsets for tabstops since it gets initial (before the whole transaction) ranges as the input. I think that the only way to actually make this cleaner would be to allow change_by_selection (or its replacement that you are building) to also return the new selection, but that selection would still be relative to the initial selection. Not sure if that will make anything cleaner since you are effectively moving this offset computation logic from generate_transaction_from_snippet into change_by_selection_new

[andriigrynenko]

I think the fundamental problem is actually with the Transaction::change. It requires the replacement text in each change to be computed before the previous change is applied to the text. This creates problems for computing offsets right now, but it will also prevent us from properly rendering variables within a snippet that e.g. depend on line number. I don't see https://github.com/helix-editor/helix/pull/5728/files fixing that right now.

Basically to have a proper long-term solution, we actually need to only render the snippet for the next selection, once the snippet for the previous selection was both rendered and applied. I think the whole Transaction concept is getting in the way here, since to even be able to create a Transaction we need to keep applying that transaction as we go, and if in the end we already have a document with the transaction applied, why not just keep that rather than keeping a ChangeSet (that still only makes sense in the context of a given text that it was rendered for)?

[andriigrynenko]

The problem that I described above with the line/column position is actually reproducible even right now with multi-line snippets, starting with fn[] fn[] and triggering completion (fn). The snippet for the second cursor is rendered assuming it's going to be on column 4, while after applying snippet to the first cursor, the position of the second cursor is actually at column 3.

Let me see if I can re-work the logic to actually not rely on any existing Transaction primitives, since this multi-line example provides a good enough test case even without variables.

[pascalkuthe]

I am still working on #5728 and haven't pushed my changes yet. The details are indeed tricky to get right but it can be done with transaction it just requires tracking state while generating the transaction. Inf act the transacitons are so foundational to helix that doing anything without them is basically impossible and would amount to a huge refactor (and the transaction are actually really good design that we wouldn't want to move away from).

You are right that my comment above was a bit misleading/slightly incorrect, your approach of mapping a fixed absolute position is mostly ok (although I would map the start instead of the end and not use a selection for that) but I take issue with just reusing and manually mapping the transaction like that. With #5278 as I have it locally applying a transaction can drop some changes and change the selection and that means that just mapping some positions won't work correctly.

Just wait a bit till I finish my changes, I wouldn't want to cause duplicate work for you.

[pascalkuthe]

I finished my work in #5728, feel free to take a look, I feel that my explanations are never as insightful as the code itself. I do partially apply the changeset to obtain the offset but I think I already know a way to optimize that in the future.

As you will see while I do keep tabstops relative, I map the anchor to which tabstops are relative eagerly instead of using the transaction as you endup running into a ton of edgecases otherwise

[andriigrynenko]

@pascalkuthe: yeah, your implementation makes sense and definitely fixes all the issues with both overlapping but also rendering each snippet knowing the right position. I'll close this PR, since your PR will fix the underlying issue once landed.

At the same time I find the new API (change_by_selection_ignore_overlapping) pretty complex and yet it's still not serving all the needs of the snippet logic:

1. You still need to map the changeset to the cursor position
2. You still need to map the changeset to the doc (in case of a multiline completion or in the future any completion that needs to know the line/column)
   Right now you pretty much have this complexity both in the change_by_selection_ignore_overlapping implementation, but also in the caller of change_by_selection_ignore_overlapping.

I'm really struggling to see why we need to go through all this trouble given that any such Transaction will be applied exactly once right after being computed.