Avoid moving the cursor when applying textedits #407
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In 33c9dc3 we removed a workaround that resets the cursor to the previous location after applying an workspaceEdit, because it interfered with auto-imports. Turns out the workaround was there for a reason: When we apply general textedits (e.g. when we rename a variable), then we are always moving the cursor by accident, because internally (to apply a textedit) we delete the entire text range and then append the new range. Obviously this loses the cursor position, hence the reason for the original manual cursor reset workaround. Instead of reintroducing that workaround, we now avoid moving the cursor altogether: We accomplish this by no longer deleting the entire range and inserting it again. Instead we just remove lines that actually need to be deleted, add lines that actually need to be added and then handle the rest with a normal setbufline() call. This will cause the cursor to never lose the correct position, because we never remove the entire region where the cursor is located in. We have to be careful with off-by-one errors, therefore this also adds an extra test for the scenario where we have to delete some lines. The two other scenarios already had comprehensive tests (adding lines, and keeping the amount of lines).
vimpostor
force-pushed
the
textedit_cursor
branch
from
6c69610 to
fbe9e8c
Compare
|
@yegappan not sure if had outage, but so far i felt this is a good refine, if you had no objection, perhaps good to merge. // @vimpostor |
|
Thanks for the patch and the detailed description. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently the
ApplyTextEdits()code loses the cursor position, because it applies the text edits by deleting the entire range withdeletebufline()and then adding the new range withappendbufline().Obviously the cursor will move, if it happens to be inside this range, which is often the case (e.g. when renaming a variable).
This is also apparently the reason, why in the past there was a workaround that saved the cursor position before applying any edits, and then restored it afterwards.
That workaround was already (correctly) removed with: #389 (comment)
Instead of reintroducing the workaround, this patch avoids losing the cursor position altogether by not deleting the entire range, but only deleting lines that actually need to be deleted (and adding new lines that actually need to be added).
In my opinion this solution is even simpler code-wise than it was before where we removed the entire region.
I also added a new test case for the scenario where lines have to be removed, all other cases already have test cases.
Demo
The old behaviour is on the left side: After renaming the variable, the cursor jumps to the end of the range.
The new behaviour is on the right side: After renaming the variable, the cursor does not move.
demo.mp4