Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tbruyelle
Copy link

@tbruyelle tbruyelle commented Apr 25, 2024

When multiple calls to the apply_text_edits() function are made, the changes made to the current buffer are discarded if the following text_edit targets 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 has already been made to the current buffer before that, that change is discarded (definition of edit!).

The fix uses of a different logic: if _switch detects that the target is not in the buffer list, it calls badd before switching to the buffer that has just been added. No more edit!, no more discarded changes.

The added test will fail without the patch in the _switch function.

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.
@tbruyelle
Copy link
Author

Hey @prabirshrestha, could you please review this PR ?
As mentioned in the body, the test can reproduce the bug, so if you want to make sure that this bug exists, just pull and run the test, without the fix.
Thanks in advance 🙏

@@ -2,6 +2,7 @@ function! s:set_text(lines)
% delete _
put =a:lines
execute 'normal ggdd'
execute 'write! /tmp/xyz'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line needed?

Copy link
Author

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.

Copy link
Author

@tbruyelle tbruyelle May 7, 2024

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattn I have changed to file my-file which has the benefit of non affecting the file system.
7fce3b87fce3b8

@tbruyelle
Copy link
Author

Hey there, any chance to have a review for this PR plz ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants