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

nvim_buf_set_lines adjustment of cursor and of topline is inconsistent #27720

Closed
pysan3 opened this issue Mar 3, 2024 · 18 comments · Fixed by #27815 or #27854
Closed

nvim_buf_set_lines adjustment of cursor and of topline is inconsistent #27720

pysan3 opened this issue Mar 3, 2024 · 18 comments · Fixed by #27815 or #27854
Labels
bug-regression wrong behavior that was introduced in a previous commit (please bisect)
Milestone

Comments

@pysan3
Copy link

pysan3 commented Mar 3, 2024

Problem

local lines = vim.api.nvim_buf_get_lines(0, 0, -1, false)
vim.api.nvim_buf_set_lines(0, 0, #lines, true, lines)

I believe the above code should do nothing to the buffer, but with the latest nightly, buffer is scrolled downwards resulting an inconsistent user experience.

I was not able to bisect the exact commit causing this problem, but I'm pretty sure this was caused by the series of refactoring #24889 done by @bfredl .

I found several similar issues, but they referred to the issue with some plugin or lsp.format, whereas my steps to reproduce is reproducible without any setup (nvim -u NONE).

Steps to reproduce

No config file required. (nvim -u NONE)

  1. Open a file that has more lines than nvim's window height.
  2. G to go to the bottom.
  3. H to move the cursor to the top of the window.
  4. Run the following code.
local lines = vim.api.nvim_buf_get_lines(0, 0, -1, false)
vim.api.nvim_buf_set_lines(0, 0, #lines, true, lines)

Result

The buffer and cursor moves down and lands one line above the bottom scrolloff. The exact same behavior as doing zb after step 3..

Expected behavior

Do not scroll the buffer, and have no visual change in above case.

Neovim version (nvim -v)

NVIM v0.10.0-dev-2504+g3971797be1

Vim (not Nvim) behaves the same?

N/A

Operating system/version

Linux

Terminal name/version

Wezterm

$TERM environment variable

tmux-256color

Installation

Build from source

@pysan3 pysan3 added the bug issues reporting wrong behavior label Mar 3, 2024
@zeertzjq zeertzjq added bug-regression wrong behavior that was introduced in a previous commit (please bisect) and removed bug issues reporting wrong behavior labels Mar 3, 2024
@bfredl
Copy link
Member

bfredl commented Mar 4, 2024

I believe the above code should do nothing to the buffer, but with the latest nightly, buffer is scrolled downwards resulting an inconsistent user experience.

It is/was not, and it cannot be true in general that this code does nothing to the buffer.

It is a valid open question how it should scroll though. Just as with the other issue, a difference to 0.9.5 by itself does not prove a bug, but it shows a case where it can be discussed if the previous or the current behavior is the one we want moving forward.

@pysan3
Copy link
Author

pysan3 commented Mar 4, 2024

@bfredl Thank you for your response. I am glad to see that this issue has been caught on your radar.

I understand that preserving the cursor position while rewriting the buffer content can be challenging. However, I believe it is important to maintain the previous behavior where the cursor position relative to the buffer and window remains the same.

A common scenario is when reformatting a file, where the entire buffer is replaced with for example stylua's output.

It is quite frustrating to have the cursor constantly moving with each format, making the editing experience very unpleasant.

I have also encountered this issue with UI plugins, such as nui.nvim, where the cursor jumps when the tree is redrawn inside the buffer, even when the content has not changed.

@pysan3 pysan3 changed the title BUG: nvim_buf_set_lines scrolls the buffer. BUG: nvim_buf_set_lines scrolls the buffer Mar 4, 2024
@pysan3
Copy link
Author

pysan3 commented Mar 4, 2024

I believe the above code should do nothing to the buffer, but with the latest nightly, buffer is scrolled downwards resulting an inconsistent user experience.

It is/was not, and it cannot be true in general that this code does nothing to the buffer.

It is a valid open question how it should scroll though. Just as with the other issue, a difference to 0.9.5 by itself does not prove a bug, but it shows a case where it can be discussed if the previous or the current behavior is the one we want moving forward.

@bfredl Could you please share with me what other behavior you are thinking could be a viable option? I cannot think of any other acceptable outcome and I think what I described should be the way nvim_buf_set_lines should behave.
I consider this a bug when nvim_buf_set_lines moves the cursor position.

@bfredl
Copy link
Member

bfredl commented Mar 4, 2024

Could you please share with me what other behavior you are thinking could be a viable option?

The current one, which matches the actual semantics of set_lines replacing the entire range of lines with another range of lines. and then for neovim to provide some other mechanism for plugins to say that line numbers are at least "almost" preserved in this operation (i e it is fine to approximate old line numbers with new ones to keep the view almost the same in windows, current or not).

The key point here is to relay the semantics of the operation, and nvim_buf_set_lines doesn't do that by itself.

@pysan3
Copy link
Author

pysan3 commented Mar 4, 2024

@bfredl Please correct me if I'm misinterpreting but I'm not complaining about changes in line number. In fact, line number of cursor mostly stays same from before to after nvim_buf_set_lines which is awesome.

My issue is that the cursor position relative to the window changes after the operation.

As you can see in the record, I start from the 6th line inside the window, but after running nvim_buf_set_lines, the cursor position jumps to somewhere around the middle of the screen (and I think the position is undetermined?), where the expected behavior is to keep the cursor on the 6th line (inside the window) AND on line 73 (inside the file).

record.mp4

@pysan3 pysan3 changed the title BUG: nvim_buf_set_lines scrolls the buffer BUG: nvim_buf_set_lines scrolls the window Mar 4, 2024
@pysan3
Copy link
Author

pysan3 commented Mar 5, 2024

Does this make sense to you @bfredl ?

@bfredl
Copy link
Member

bfredl commented Mar 5, 2024

yes, it is a bug that cursor position is not handled consistently like topline. I made the mistake of assuming that the title of the issue would describe the issue:)

@bfredl bfredl changed the title BUG: nvim_buf_set_lines scrolls the window nvim_buf_set_lines adjustment of cursor and of topline is inconsistent Mar 5, 2024
@pysan3
Copy link
Author

pysan3 commented Mar 7, 2024

Thanks for your reply @bfredl !

English is not my native language so forgive me about that :P

Were you able to reproduce the issue?

Do you have any rough estimate on when this can be fixed?

@bfredl bfredl added this to the 0.10 milestone Mar 7, 2024
@bfredl
Copy link
Member

bfredl commented Mar 7, 2024

yes, it will be fixed before 0.10 .

@pysan3
Copy link
Author

pysan3 commented Mar 7, 2024

@bfredl Thanks. Hope it gets fixed as soon as possible.

FYI, this issue might be caused by the same core problem.

bfredl added a commit to bfredl/neovim that referenced this issue Mar 11, 2024
A lot of functions in move.c only worked for curwin, alternatively
took a `wp` arg but still only work if that happens to be curwin.

Refactor those that are needed for update_topline(wp) to work
for any window.

fixes neovim#27723
fixes neovim#27720
bfredl added a commit to bfredl/neovim that referenced this issue Mar 11, 2024
A lot of functions in move.c only worked for curwin, alternatively
took a `wp` arg but still only work if that happens to be curwin.

Refactor those that are needed for update_topline(wp) to work
for any window.

fixes neovim#27723
fixes neovim#27720
bfredl added a commit to bfredl/neovim that referenced this issue Mar 11, 2024
A lot of functions in move.c only worked for curwin, alternatively
took a `wp` arg but still only work if that happens to be curwin.

Refactor those that are needed for update_topline(wp) to work
for any window.

fixes neovim#27723
fixes neovim#27720
bfredl added a commit to bfredl/neovim that referenced this issue Mar 11, 2024
A lot of functions in move.c only worked for curwin, alternatively
took a `wp` arg but still only work if that happens to be curwin.

Refactor those that are needed for update_topline(wp) to work
for any window.

fixes neovim#27723
fixes neovim#27720
bfredl added a commit to bfredl/neovim that referenced this issue Mar 11, 2024
A lot of functions in move.c only worked for curwin, alternatively
took a `wp` arg but still only work if that happens to be curwin.

Refactor those that are needed for update_topline(wp) to work
for any window.

fixes neovim#27723
fixes neovim#27720
bfredl added a commit to bfredl/neovim that referenced this issue Mar 11, 2024
A lot of functions in move.c only worked for curwin, alternatively
took a `wp` arg but still only work if that happens to be curwin.

Refactor those that are needed for update_topline(wp) to work
for any window.

fixes neovim#27723
fixes neovim#27720
bfredl added a commit to bfredl/neovim that referenced this issue Mar 12, 2024
A lot of functions in move.c only worked for curwin, alternatively
took a `wp` arg but still only work if that happens to be curwin.

Refactor those that are needed for update_topline(wp) to work
for any window.

fixes neovim#27723
fixes neovim#27720
bfredl added a commit to bfredl/neovim that referenced this issue Mar 12, 2024
A lot of functions in move.c only worked for curwin, alternatively
took a `wp` arg but still only work if that happens to be curwin.

Refactor those that are needed for update_topline(wp) to work
for any window.

fixes neovim#27723
fixes neovim#27720
bfredl added a commit to bfredl/neovim that referenced this issue Mar 12, 2024
A lot of functions in move.c only worked for curwin, alternatively
took a `wp` arg but still only work if that happens to be curwin.

Refactor those that are needed for update_topline(wp) to work
for any window.

fixes neovim#27723
fixes neovim#27720
bfredl added a commit to bfredl/neovim that referenced this issue Mar 12, 2024
A lot of functions in move.c only worked for curwin, alternatively
took a `wp` arg but still only work if that happens to be curwin.

Refactor those that are needed for update_topline(wp) to work
for any window.

fixes neovim#27723
fixes neovim#27720
bfredl added a commit to bfredl/neovim that referenced this issue Mar 12, 2024
A lot of functions in move.c only worked for curwin, alternatively
took a `wp` arg but still only work if that happens to be curwin.

Refactor those that are needed for update_topline(wp) to work
for any window.

fixes neovim#27723
fixes neovim#27720
bfredl added a commit to bfredl/neovim that referenced this issue Mar 13, 2024
A lot of functions in move.c only worked for curwin, alternatively
took a `wp` arg but still only work if that happens to be curwin.

Refactor those that are needed for update_topline(wp) to work
for any window.

fixes neovim#27723
fixes neovim#27720
@bfredl bfredl closed this as completed in 08fc1eb Mar 13, 2024
@bfredl
Copy link
Member

bfredl commented Mar 13, 2024

Please try (very) latest master, just merged a fix.

@pysan3
Copy link
Author

pysan3 commented Mar 13, 2024

@bfredl Unfortunately, I don't think it is fixed.

I've tried to detect the pattern of the behavior but I'm not really sure.
(I'm sometimes using <C-e>, <C-y> or the mouse to scroll.)

Do you want me to open a new issue, or may I ask some maintainer reopen this thread?

record.mp4

@pysan3
Copy link
Author

pysan3 commented Mar 13, 2024

On top of that, I'm getting segfault crashes from commit 08fc1eb. I cannot detect how or when the crash is happening, but it is happening everytime I open a second buffer.

This is not reproducible with nvim -u NONE but it gets fixed if I move back one commit prior so there should be a minor edge case in your code. I'll try to detect the problem, but at least reporting it now to let you know.

image

Here is the stack trace (I masked some info).
bt.txt

The most suspecious part IMO is this part. Hope this info helps, and please guide me what I should do if you need more information. I'm not very familiar with C but I'm more than happy to help.

Program terminated with signal SIGSEGV, Segmentation fault.
#0  changed_window_setting (wp=0x0) at /home/------/.local/share/nvim-git/src/nvim/move.c:541
541	  wp->w_lines_valid = 0;

@bfredl bfredl reopened this Mar 13, 2024
@zeertzjq
Copy link
Member

Is that the full stacktrace you got by typing bt?

@bfredl
Copy link
Member

bfredl commented Mar 14, 2024

@pysan3 would you mind trying #27854 and see if behaves like you expect?

@pysan3
Copy link
Author

pysan3 commented Mar 14, 2024

@bfredl Amazing! Your branch boogalo_lines works as expected.

However I'm still having a segfault issue with my own config as I reported above.

Here is the full track trace. I managed to figure out how to get the full stack @zeertzjq :)

bt.txt

I'm still not able to reproduce this with nvim -u NONE but I think it happens when you have set a window option in a autocmd? I'm not really sure. I hope the stack trace gives you enough information.

@seandewar
Copy link
Member

Looking at the bt, it looks like you have some plugin calling changed_window_setting via the FFI, but it's assuming the old signature (without the wp argument, as it's NULLed out).

Looking at your plugins, it's probably nvim-ufo, which updated the signatures yesterday; you'll need to update it.

@pysan3
Copy link
Author

pysan3 commented Mar 14, 2024

@seandewar THANK YOU!! You saved my day!

I can confirm that it was fixed by the latest commit in nvim-ufo (latest tag/release is too old tho).
@bfredl @zeertzjq Thanks for digging in. It was a plugin related issue and it seems to be fixed now. I'm sorry it wasn't a direct neovim issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-regression wrong behavior that was introduced in a previous commit (please bisect)
Projects
None yet
4 participants