-
Notifications
You must be signed in to change notification settings - Fork 305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(text_edit): discarded change from the initial buffer #1552
fix(text_edit): discarded change from the initial buffer #1552
Conversation
When multiple calls to apply_text_edits, the changes made to the current buffer are discarded if the following text_edit concerns a file that is not in the buffer list. The problem comes from the _switch function, which executes `edit!` when it detects that the target is not in the buffer list. If a change was made to the current buffer before that, that change is discarded (definition of `edit!`). The fix consists of a different logic: when _switch detects that the target is not in the buffer list, it calls `badd` prior to switching to the buffer that has just been added. No more `edit!`, no more discarded changes. The added test fails without the patch in the _switch function.
Hey @prabirshrestha, could you please review this PR ? |
test/lsp/utils/text_edit.vimspec
Outdated
@@ -2,6 +2,7 @@ function! s:set_text(lines) | |||
% delete _ | |||
put =a:lines | |||
execute 'normal ggdd' | |||
execute 'write! /tmp/xyz' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this line needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because otherwise the buffer created by this s:set_text()
function will have no name. Because the _switch(path)
function is called with the return value of bufname('%')
, if it gets an empty path
it doesn't work properly.
This wasn't discovered before because no tests before the one I added actually pass more than one text_edits
to the lsp#utils#text_edit#apply_text_edits(uri, text_edits)
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said there's maybe a better way to give a name to a buffer, without involving a disk write.
For example I can replace that line with execute 'file xyz'
. The tests are still green after that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there, any chance to have a review for this PR plz ? |
Merged. Thanks. |
When multiple calls to the
apply_text_edits()
function are made, the changes made to the current buffer are discarded if the followingtext_edit
targets a file that is not in the buffer list.The problem comes from the
_switch
function, which executesedit!
when it detects that the target is not in the buffer list. If a change has already been made to the current buffer before that, that change is discarded (definition ofedit!
).The fix uses of a different logic: if
_switch
detects that the target is not in the buffer list, it callsbadd
before switching to the buffer that has just been added. No moreedit!
, no more discarded changes.The added test will fail without the patch in the
_switch
function.