fix(renderer): Makes position.save() from render_tree a lower priority

[georgeguimaraes]

This fixes some issues when several events trigger render_tree, and you open/browse a lot of files and directories.

A race condition happens when a position is saved but never restored.

I'm not super happy with the addition of another conditional. Suggestions are always welcomed.

Fixes: #1310

[cseickel]

Thanks for tackling this. I get the general concept but could use more details.

Could you explain what position save are overwritable vs what is not and what are the sequence of events that caused you to make these choices?

[georgeguimaraes]

Sure,

1. We need to save and restore the position in render_tree because of the changes in refactor(change): do API changes to buffer without curbuf switch neovim/neovim#24824.

2. We also need to save the position when we close neo-tree's window in case the user runs :Neotree show (without follow_current_file). That's what my PR fix(renderer): fix Neotree focus/show toggle behaviour #1288 meant to fix. We can rely on the fact that because of 1), render_tree will restore the position, but we don't want the M.position.save() from inside render_tree to overwrite the M.position.save() from when we close the window, that's why the guard clause was added.

Those are the 2 happy paths I was dealing with.

However, there is a race condition that it's difficult for me to detect in my machine. It appears that two events (vim_buffer_enter and vim_buffer_added, but I'm not entirely sure) trigger render_tree concurrently, and one tries to save the save position but can't. The problem is that we don't restore it and thus, we don't reset the position with nil. And now, the next open/expand command in neo-tree window will restore the window to a stale position.

I've considered using a queue here, but out-of-order messages would cause the same problem.

Since saving the position in render_tree is actually a workaround for a neovim behavior and not a feature, we can overwrite it if there's another part of the code that wants to save a position.

TL;DR:
M.position.save() from render_tree is always overwritable. In other places, it shouldn't be.

Thoughts?

[mehalter]

@georgeguimaraes I'm able to periodically trigger this race condition still while browsing around in neo-tree. I do think it's happening less frequently than before but still present

[XavierChanth]

@georgeguimaraes I've been experiencing the same issue as @mehalter, I am able to produce #1310 consistently when running your branch:

My neo-tree config

My config is a LazyVim base:
https://github.com/LazyVim/LazyVim/blob/a50f92f7550fb6e9f21c0852e6cb190e6fcd50f5/lua/lazyvim/plugins/editor.lua#L6-L101

With the following customizations layered on top:

return {
  "georgeguimaraes/neo-tree.nvim",
  branch = "gg-fix-multiple-saves",
  -- "nvim-neo-tree/neo-tree.nvim",
  -- tag = "3.14",
  opts = {
    window = {
      mappings = {
        ["w"] = {
          function(state)
            local current_node = state.tree:get_node()
            local path = current_node:get_id()

            -- If you want to set the root as well
            -- require("neo-tree.sources.filesystem.commands").set_root(state)
            vim.cmd("cd " .. path)
            vim.print("cwd: " .. path)
          end,
          desc = "set working directory",
        },
      },
    },
  },
}

I was using the <leader>e mapping to open/close neo-tree.
Which is a remap to:

  require("neo-tree.command").execute({ toggle = true, dir = Util.root() })

[georgeguimaraes]

Back to the drawing board, then. I'll try another approach.
Thanks, @mehalter and @XavierChanth, for trying it.

[XavierChanth]

Back to the drawing board, then. I'll try another approach. Thanks, @mehalter and @XavierChanth, for trying it.

Thanks, feel free to ping me as a tester

[mehalter]

Back to the drawing board, then. I'll try another approach.
Thanks, @mehalter and @XavierChanth, for trying it.

No problem at all! Thanks so much for spending time on this @georgeguimaraes 😊

[pysan3]

@mehalter Could you give me some examples of this problem and how I can test this?

I'm doing a big rewrite and I might be able to deal with async updates of tree using mutex from nvim-nio (i.e. semaphore(1)).

[mehalter]

@pysan3 wow that discussion thread looks amazing, and I am super excited to see what you put together! I'll be honest and say I don't fully understand the saving/restoring procedure happening here, like why it is necessary or why there is a race condition. At least no more than the incredible comment description above by @georgeguimaraes (#1354 (comment)) . They might be able to explain better/more in detail if you need that.

Where I am producing the issue with when using the hjkl based navigation code proposed in this discussion thread: #163

There is a minimal repro.lua file in a comment in the linked issue above (#1310 (comment)). Don't worry about the comment saying that it doesn't reproduce the issue, it does. The weird part is because it's a race condition it is relatively non-deterministic on whether or not the behavior appears and when. But when havingating using only the hjkl navigation opening files and opening/closing directories specifically with the h and l keys will sometimes mess up where the cursor location in neo-tree gets placed. There is a video of this behavior linked in the original issue as well that might be clearer than my explanation 😅

When replicating, I basically just frantically move about the file system using hjkl in neo-tree and see if it randomly jumps to a previously opened directory. Sorry if this isn't super great replication instructions 😞 Let me know if you have any questions that I can try and clear up!

Let me know if you want any help testing/working on your new refactor!

[pysan3]

Oh I missed the video link. Your explanation is awesome.

So, in the video, I believe this tree is opened with reveal=true, so the expected behavior would be;

• expand event/ -> cursor stays on event
• go back to the left window -> neo-tree focus goes back to layout.tsx
• reenter neo-tree -> cursor on layout.tsx
• move down to event/
• close event/ -> cursor stays at event

right?

I believe the original video is more of an issue with :Neotree show + follow file vs user manually expanding some other location.
(cursor pos after follow file will be remembered, but when user comes into neo-tree window and move the cursor manually, that change in position is not captured (which should be force updated right before keybind is executed, but is not) and on the next rerender, cursor goes back to the current file) (but this explanation is old, it should be fixed by whatever PR that I don't remember).

And I think your issue is that if you expand two different directories simultaneously (or just move the cursor after commanding an expand but before it completes the rerender), the cursor goes back to the first directory, where you would want the cursor to be on the final location and don't mess with my cursor right?

@mehalter

[mehalter]

So the video is not showing any issues with the open file following the cursor actually. It is showing that the user moved the cursor down in the neo-tree window, presses l to open the directory now under the cursor (with the custom binding), and upon opening the directory their cursor was ("restored") moved up to a previous location rather than staying in the newly open directory.

Here is the video I was referring to from the issue just in case there may have been another one somewhere that maybe wasn't correct (error happens between second 4 and 5): https://photos.onedrive.com/share/F80D48919F327BE9!32352?cid=F80D48919F327BE9&resId=F80D48919F327BE9!32352&authkey=!AI-hSiBNabNE5j4&ithint=video&e=IeXjS9

It appears this is happening because of some stale saved information which "restores" the cursor but because it is stale it restores it to a previous, incorrect location.

[pysan3]

Sorry I updated my comment. I was too late ig?

(cursor pos after follow file will be remembered, but when user comes into neo-tree window and move the cursor manually, that change in position is not captured (which should be force updated right before keybind is executed, but is not) and on the next rerender, cursor goes back to the current file) (but this explanation is old, it should be fixed by whatever PR that I don't remember).

[pysan3]

What you are trying to fix is more of a race condition thing right?

And I think your issue is that if you expand two different directories simultaneously (or just move the cursor after commanding an expand but before it completes the rerender), the cursor goes back to the first directory, where you would want the cursor to be on the final location and don't mess with my cursor right?

---

I think the video bug can be reproduced even when done slowly.

*can could, isn't it fixed by now?

[mehalter]

yeah, the race condition is not because 2 actions are being taken by the user too quickly. It's just inherent in the code. It's not a race condition because the user is moving too quickly

[mehalter]

the bug in the video is the one that this PR is trying to fix and doesn't. I reproduce it by navigating frantically just to increase the number of operations I am doing so I don't spend a long time trying to reproduce the bug 😅 I may have communicated this poorly. It isn't dependent on how fast the user is moving, just how much time they want to spend trying to move, open directories, close them, move, etc. But the speed of the user is not a part of the issue :D

[mehalter]

The main thing is the behavior in the video doesn't happen 100% of the time. It happens non-deterministically (indicating a race condition in the underlying code). So there aren't a set of steps I can give you to reproduce the bug 100% of the time. I couldn't give you like a set of hjkl operations that if you did within a time frame would reproduce the bug. It's like, it could happen the first time you open and close a directory, it could happen the 100th time you open and close a directory

[pysan3]

Huh, that's weird.

What is the command when opening neo-tree? what is reveal? show? focus?

[mehalter]

ok maybe I should just clear this up because I think you think this bug is related to the cursor following the open file. But it is completely unrelated. One sec. I will do a recording

[pysan3]

Thanks a lot. Sorry to bother your time...

[XavierChanth]

@pysan3 my issue is slightly different than @mehalter but it seems to be related. I was able to reproduce consistently, but only for the first folder opened each time I toggle.

Updated my message above with the full config + the specific keybind / command I was using. Note that I put my config in a toggle block since it is quite long.

[mehalter]

@pysan3 https://asciinema.org/a/TRr73ID4EOsEQ38IQJfquGV9v

This video shows that I opened neo-tree with :Neotree. That is the only neo-tree command issued. After that I was only navigating with h,j,k,l and between windows with <C-w>h and <C-w>l.

Also No worries at all! I am sorry it is hard to explain, especially because I'm also not 100% certain on my own understanding of the issue 😅

It also might be important to note that this is the commit that introduces the race condition: #1288. Before this, there is no issue at all.

[pysan3]

Thank you guys that is awesome @XavierChanth @mehalter

@XavierChanth
However I can't see your cursor in asciinema and I can't quite follow what you are doing. Could you give me instructions (keybinds)?

@mehalter
Ok I was able to reproduce, but only when config.filesystem.follow_current_file.enabled = true which is not in repro.lua in the old comment. Am I right?

[mehalter]

@pysan3 I can reproduce this without the follow_current_file enabled: https://asciinema.org/a/wECW01iEk1zmnZHlNf7tJX1rM

repro.lua

-- DO NOT change the paths and don't remove the colorscheme
local root = vim.fn.fnamemodify("./.repro", ":p")

-- set stdpaths to use .repro
for _, name in ipairs({ "config", "data", "state", "cache" }) do
  vim.env[("XDG_%s_HOME"):format(name:upper())] = root .. "/" .. name
end

-- bootstrap lazy
local lazypath = root .. "/plugins/lazy.nvim"
if not vim.loop.fs_stat(lazypath) then
  vim.fn.system({ "git", "clone", "--filter=blob:none", "https://github.com/folke/lazy.nvim.git", lazypath })
end
vim.opt.runtimepath:prepend(lazypath)

-- install plugins
local plugins = {
  "folke/tokyonight.nvim",
  -- add any other plugins here
}

local neotree_config = {
  "nvim-neo-tree/neo-tree.nvim",
  dependencies = { "MunifTanjim/nui.nvim", "nvim-tree/nvim-web-devicons", "nvim-lua/plenary.nvim" },
  cmd = { "Neotree" },
  keys = {
    { "<Leader>e", "<Cmd>Neotree<CR>" }, -- change or remove this line if relevant.
  },
  opts = {
    log_level = "trace",
    log_to_file = "/tmp/neotree-repro.log",
    filesystem = {
      filtered_items = {
        visible = true,
        show_hidden_count = true,
        hide_dotfiles = false,
        hide_gitignored = true,
        hide_by_name = {
          -- '.git',
          -- '.DS_Store',
          -- 'thumbs.db',
        },
        never_show = {},
      },
      window = {
        mappings = {
          h = function(state)
            local node = state.tree:get_node()
            if (node.type == "directory" or node:has_children()) and node:is_expanded() then
              state.commands.toggle_node(state)
            else
              require("neo-tree.ui.renderer").focus_node(state, node:get_parent_id())
            end
          end,
          l = function(state)
            local node = state.tree:get_node()
            if node.type == "directory" or node:has_children() then
              if not node:is_expanded() then -- if unexpanded, expand
                state.commands.toggle_node(state)
              else -- if expanded and has children, seleect the next child
                require("neo-tree.ui.renderer").focus_node(state, node:get_child_ids()[1])
              end
            else -- if not a directory just open it
              state.commands.open(state)
            end
          end,
        },
      },
    },
    window = {
      position = "right",
    },
  },
}

table.insert(plugins, neotree_config)
require("lazy").setup(plugins, {
  root = root .. "/plugins",
})

vim.cmd.colorscheme("tokyonight")
-- add anything else here

[mehalter]

@pysan3 here is a replication using purely just the repro.lua given by csiekel in this comment: #1310 (comment)

https://asciinema.org/a/GHLL4T2FKwq4MJhGyYpmf72JK

So this doesn't even use changes in my implementation of the l command.

[mehalter]

@georgeguimaraes while making these recordings I did find that I can replicate this issue 100% of the time actually!

Steps (with repro.lua from here: #1310 (comment)):

1. nvim - open Neovim
2. :Neotree - open neo-tree
3. Open a directory with l
4. <C-w>h - switch to the other window
5. <C-w>l - switch back into the neo-tree sidebar
6. Navigate to another closed directory and run l
7. See that the cursor jumps to the wrong location

Demo: https://asciinema.org/a/PGmF1DoW1X36FVbz1Ps0ATNzB

[mehalter]

Also worth noting the above steps reproduce the bug even with this PR

[pysan3]

@mehalter Oh you are right. I'm now on the same page.

And now I understand why you added save(..., overwritable) and that it doesn't solve the issue kekw.

OK, let me try to tackle the problem ;)

[pysan3]

Hey @mehalter, could you test my fix? I think this should solve the issue.

cc @georgeguimaraes @XavierChanth @cseickel

https://github.com/pysan3/neo-tree.nvim/tree/update-curpos-before-expand

{ 
  "pysan3/neo-tree.nvim",
  branch = "update-curpos-before-expand",
  version = false,

Sidenote, the branch name is completely wrong lol. I made the branch when I was not understanding the problem and the actual fix of this problem is not what you think ;)
pysan3@a9d5ee5

[mehalter]

@pysan3 I believe that does fix it! You probably have a better understanding of why it works and if it is necessary. But my first concern/question would be if that introduces other bugs when it comes to restoring. I am just wondering what led to those pieces of code being put in in the first place if they were to fix some bugs and if they are still fixed when removing the position saving. Thanks so much for looking at this for reals!!

[pysan3]

I had to re-add BufDelete aucmd since user might force close neo-tree with :q or :bd.

Since the block was added to not overwrite saved position, we now need to be exactly sure that previous position must be restored and flushed (set to nil) before when we really want to save (otherwise, there will be a position remaining when you really want to save the position).

Position saved at WinLeave will not be restored at any moment and that's the reason of the issue. What seems like a solution is to add WinEnter to restore, but as it is said in the comment, WinEnter is too early to restore because render_tree might not be finished.

The reason why WinLeave autocmd was added in the first place, was that this was the only place saving the position when this whole position saving mechanism was added. But this was a problem and people started adding save at the right locations such as after render_tree and before close. Other than these situations + BufDelete, I don't think there's any place where vim looses cursor position but neo-tree wants to restore it.

Man it's pretty difficult to explain in words...
(that's why I'm on a rewrite to make the code a lot cleaner)

[pysan3]

I'll wait for either @georgeguimaraes or @XavierChanth or any other person that previously had this issue, and if you give me a go, I'll make a PR.

You know that I didn't have this issue before so I can't test it with my workflow / config options lol.

[XavierChanth]

However I can't see your cursor in asciinema and I can't quite follow what you are doing. Could you give me instructions (keybinds)?

Sorry, I didn't realize that was the case. I'll test your branch right now, and if I still have this issue then I'll rerecord and explain.

[XavierChanth]

I'll wait for either @georgeguimaraes or @XavierChanth or any other person that previously had this issue, and if you give me a go, I'll make a PR.

Seems to be less reproducible, but still happening...

https://asciinema.org/a/idz4OwuEX6TteaZK7NkpfSxjZ

My cursor only seems to jump when I toggle open a folder. In both of my recordings, I'm using <leader>+e to toggle neotree opened / closed.

Im using j/k to navigate vertically in neotree, and I'm using <CR> to toggle the directories opened/closed.

It doesn't seem to happen when I close directories, only open them.
In the recording, you see me often toggle directories closed, then toggle neotree open and closed. For me, the issue is only reproducible right after toggling neotree open, and it seems to only jump on the first directory toggled open after opening neotree (so I have to close directories and neotree to reset again).

[pysan3]

Thanks mate!

Ok let me dig into the problem. It'll probably be in the weekends and I'll make a PR when I'm done.

[cseickel]

Closing because I think this was fixed by #1355, let me know if that's wrong.