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(renderer): fix cursor jumping #1377

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

pysan3
Copy link
Collaborator

@pysan3 pysan3 commented Mar 4, 2024

Originated from: #1374

Discussion

I reported this before, but it's not completely fixed: #996

I tracked it down to this piece of codes:

execute_win_command("normal! zb")
local top = vim.fn.line("w0", state.winid)
local bottom = vim.fn.line("w$", state.winid)
local offset_top = top + (expected_bottom_line - bottom)
execute_win_command("normal! " .. offset_top .. "zt")
pcall(vim.api.nvim_win_set_cursor, state.winid, { linenr, col })

If I comment it out, everything works perfectly as expected.

Anybody knows what that piece of code does?

execute_win_command("normal! zb")

This moves the current node at the bottom of screen.

execute_win_command("normal! " .. offset_top .. "zt")

Then this moves another line at the top of screen.

These causes unnecessary jumps and it's very problematic.

Originally posted by @MunifTanjim in #1374


zb moves your cursor to the bottom of the window, respecting scrolloff
<line>zt moves the cursor to the indicated line and sets that line as the top of the window, respecting scrolloff

Why?

This chunk of code is not about saving the top line at all, it is the opposite. It moves the top line if either of these situations occur:

  1. The window is scrolled down even though the entire tree can now fit in the window without scrolling.
  2. The focused node is scrolled off of the screen, after we take into account the requested extra padding

Fixing the scrolled down to far problem is mostly about opening and closing folders which change the overall size of the tree and can lead to be scrolled down when you don't need to be.

The "requested extra padding" is for the sake of the search popup which floats over top of the bottom of the window. You need the extra padding to ensure that the focused node is not under the floating window.

How?

Given that understanding, I will attempt to walk through the code.

      -- make sure we are not scrolled down if it can all fit on the screen
      local lines = vim.api.nvim_buf_line_count(state.bufnr)
      local win_height = vim.api.nvim_win_get_height(state.winid)
      local expected_bottom_line = math.min(lines, linenr + 5) + bottom_scroll_padding
      if expected_bottom_line > win_height then
        execute_win_command("normal! zb")
        local top = vim.fn.line("w0", state.winid)
        local bottom = vim.fn.line("w$", state.winid)
        local offset_top = top + (expected_bottom_line - bottom)
        execute_win_command("normal! " .. offset_top .. "zt")
        pcall(vim.api.nvim_win_set_cursor, state.winid, { linenr, col })
      elseif win_height > linenr then
        execute_win_command("normal! zb")
      elseif linenr < (win_height / 2) then
        execute_win_command("normal! zz")
      end
  • First off, it's important to note that when we get here the focused node has already been set and it can be anywhere on the screen.
  • expected_bottom_line is where we want the focused node to be if it can't be visible by scrolling the window all the way to the top, which is the ideal goal.
  • IF that expected_bottom_line is off of the screen, then:
    • normal! zb move the current focused line to the bottom of the window, preserving the user's scrolloff setting.
    • Then we take a measurement and see in this hypothetical situation where the focused node is at our ideal "bottom" of the screen, what would our top line be?
    • Now we move the cursor to that top line, respecting scrolloff. Why? I have no idea. Maybe everything after normal! zb should be removed.
    • Then we set the cursor back to where we started.

I could have misinterpreted, even though I think I did write this code.

Originally posted by @cseickel in #1374 (comment)


If I'm not mistake, all these are needed when neo-tree needs to reveal the file for current buffer in the tree, right?

What about the case when a node is manually being collapsed? That's when the weird jump occurs. Here's a recording:

Kapture 2024-03-03 at 21 23 25

If I comment out those codes, it works as expected:

Kapture 2024-03-03 at 21 22 31

Originally posted by @MunifTanjim in #1374 (reply in thread)


OK, I've looked into the code and here's the problem.

expected_bottom_line should actually be the most bottom line cursor can be in order to not conflict with bottom_scroll_padding (ie search bar height).

Current code forces cursor position to be on this line, even when there are actually more content below the cursor to spare and there were no reason to be afraid of bottom_scroll_padding.

normal! zb and other code are necessary when the tree had more height than the window and the last line was below window's bottom line, but after closing the node, the buffer length shrunk and last line is now visible. Neotree will scroll down a bit so that there are no blank lines below the tree.

Originally posted by @pysan3 in #1374 (reply in thread)


This should work all as expected.

Working nicely for me so far 🚀

Originally posted by @MunifTanjim in #1374 (reply in thread)

Reproduce

Here's how I reproduced it with the linux kernel.

  1. :Neotree reveal_file=drivers/gpu/drm/nouveau/include/nvrm/535.113.01/common/sdk/nvidia/inc/ctrl/ctrl2080/ctrl2080internal.h
  2. Cursor should be at the center of window (as if zz).
  3. close_node
  4. Cursor should be on ctrl2080/ but the window scrolled downwards as if zb (where I don't want the buffer to move).

Originally posted by @pysan3 in #1374 (reply in thread)

Mentions

I'd be more than happy if you could test this fix.

cc @cseickel @rizkyilhampra @teocns

return {
  "pysan3/neo-tree.nvim",
  branch = "fix-cursor-jumping",
  version = false,
  opts = {
    -- ...

@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 4, 2024

@cseickel I couldn't understand why you had linenr + 5 (why +5?) and I deleted that part.

Do you remember what that number was for?

@cseickel
Copy link
Contributor

cseickel commented Mar 4, 2024

@cseickel I couldn't understand why you had linenr + 5 (why +5?) and I deleted that part.

Do you remember what that number was for?

I think that I used 5 as a hard coded scrolloff because I wrote that before I learned about the scrolloff option.

@cseickel
Copy link
Contributor

cseickel commented Mar 4, 2024

@pysan3 I'm a little short on time but I'll try to test this out and review in this next 24 hours.

@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 4, 2024

Thanks a lot <3

@rizkyilhampra
Copy link

@pysan3 excuse me to write comment on this pr. thanks for pointing it out to me. but i think my problem which in #1310 (comment) is closed by v3.18. i'm also tested with your branch, but it's still produce cursor jumping when open folder. sorry i think that's all i can say, i'm not fully experience with this.

@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 5, 2024

@rizkyilhampra I really can't reproduce your issue anymore.

Could you send me a screen record?

@teocns
Copy link

teocns commented Mar 5, 2024

Pulled your branch, will report back as soon as possible. Thank you!

@MunifTanjim
Copy link
Contributor

MunifTanjim commented Mar 5, 2024

Okay, now I'm facing this too. The scrolling is not happening, but the cursor is jumping.

Kapture 2024-03-05 at 11 28 12

Commenting out these codes solves it.

@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 5, 2024

Yikes, I didn't pull the latest main branch into this branch.

This branch didn't have the fix included.

Merged this branch to the latest main branch so please test it out again @MunifTanjim @teocns .

Thanks for your reply @rizkyilhampra, indeed this branch shouldn't have worked for you lol.

Copy link
Contributor

@cseickel cseickel left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for taking care of this!

@cseickel cseickel merged commit dcb63ab into nvim-neo-tree:main Mar 6, 2024
2 checks passed
@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 9, 2024

@cseickel Looks like some amount of people are waiting for this fix.

What do you think of making a new tag release?

@cseickel
Copy link
Contributor

cseickel commented Mar 9, 2024

Released as 3.19

@pysan3 pysan3 added this to the v4.0 milestone Mar 9, 2024
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.

window is scrolled up after collapsing node
5 participants