0. Complete re-write?

[cseickel]

So @pysan3 has proposed several refactors, many of which involve breaking changes, and I'm starting to think that maybe what we should be doing is to start with a clean slate and design the perfect system, knowing what we do now. Maybe it's highly compatible with existing configs and maybe not. Either way it's better to think up the ideal first. After we have an ideal target, we can think about how to get there from where we are now. Maybe that means incremental changes and maybe it means starting from a clean slate and copying selectively from the existing code.

The problem is that even though I am proposing it and I think it is the right way to go, I don't really have the time to personally execute on this. I can do some of it though, if others are interested in this undertaking.

@pysan3
@miversen33
@nhat-vo
@ghostbuster91
@mrbjarksen

[pysan3]

Looking at the code, I don't think it's that broken, but these are the pain points.

1. the same table is referenced from many places

For example config is saved at M.config in neo-tree/setup/init.lua and also sub part is copied into state.config and so on. And also require("neo-tree").config is used in other places.

And then config in neo-tree/log.lua is a very different thing.

2. callback hell

To achieve async capability, the code in this plugin passes a very VERY nested callback hell into the functions. This basically makes it pretty hard to do type analysis in the first place, and it is very difficult to assume variable lifetime.
Due to the latter problem we cannot assume if a table is alive and we have to do the foo = foo or {} all over the place.

If you allow me to introduce nvim-nio as a dependency, we won't need any callbacks as it uses coroutines under the hood to make async code just like a normal code flow when wrapped with nio.run.

TLDR; Code wrapped inside a nio.run will not block neovim's main thread.

Self-promo Pathlib.nvim makes it even easier.

But nvim-nio does not support running a shell command asyncly (so we can't call git etc) just yet, until this PR is merged.

[miversen33]

But nvim-nio does not support running a shell command asyncly (so we can't call git etc) just yet, until nvim-neotest/nvim-nio#7 is merged.

Its worth pointing out that plenary supports this out of the box with their plenary.job interface.

Additionally, coroutines are supported with plenary.async. Is there a reason we should be considering another library that can do the same stuff as one we already are dependent on?

[pysan3]

Well, it's just my preference, but I don't quite like working with plenary cuz there's absolutely no document and I encountered bugs here and there that were very difficult to diagnose (might be fixed by now, not sure).

Plenary.job uses callbacks and I want to get rid of them.

[ghostbuster91]

I agree that plenary docs could be improved. Plenary is however battle tested in many other neovim plugins. On the other side, @pysan3 mentioned in some other thread that it supports cancellation which might be very useful.

[miversen33]

I am split on this. I have far less time in the code base than yourself, @pysan3 or @nhat-vo so take my opinion with a grain of salt here.

I am very much of the idea that "breaking change is bad" and I don't know how well we can perform a complete rewrite of neo-tree while promising compatibility with existing configurations, especially as we don't really have any sort of tests built for neo-tree.

That said, I definitely do feel pain at times when trying working on the netman integration with Neo-tree. There is alot of "magic" going on under the hood and quite a bit of that magic is not accessible to external sources. I have far less to say about maintaining neo-tree itself as for the most part I review bugs and feature PRs and don't do a ton in the way of "this hurts, we should fix it".

Additionally, neo-tree is kind of used all over the place in all kinds of configurations. Its a core component of several very popular neovim distributions and while I don't necessarily think we should develop/maintain neo-tree with that at the forefront of our mind, a complete rewrite is likely going to affect thousands of users in one way or another. I almost want to suggest a fork for that, however the fork would more or less just die (and we would end up splitting time, devs, users, etc) between the 2.

Almost like we are stuck in the "how do we make legacy code hurt less" phase of the project ;)

My personal opinion on this is that we go the approach @pysan3 is suggesting (very slowly) with the piecemeal updates/replaces of neo-tree. I think a bunch of the pain that goes into maintaining/developing/extending neo-tree can be addressed simply with better documentation (something I seem to recall agreeing to help with but never got around to really doing) both for users and developers.

Writing good documentation is hard and sucks though which is why its usually neglected lol

[pysan3]

Writing good documentation is hard and sucks though which is why its usually neglected lol

TRUE.

---

I also think we shouldn't bring any changes to the config options.

What I'd like to change is how state is handled and do async rendering without using callbacks (if that's possible).

What I might break is how event_handlers will be handled and might break external sources.

[pysan3]

The biggest problem is that we all don't know the difference between context, state, config, tree, tree.node is. And what is updated when, where and for what.

[pysan3]

function MyClass:navigate(path, path_to_reveal, ...)
  vim.print(self.id)
end

-- If and only if
-- getmetatable(state) == MyClass
MyClass.navigate = function(state, path, path_to_reveal, ...)
  vim.print(state.id)
end

Since the above code in lua means the exact same thing, it is very easy to make sources compatible with OOP without changing anything. The first notation makes it a bit easier to keep track of type annotations so I'd prefer the first style, but we can keep current sources mostly as-is thanks to the coincidence that most functions take state as the first argument.

So I'm not forcing the OOP style to all code but just using OOP where I'm doing the rewrite. (ie manager.lua and some others)

I think that fixing that problem is simpler than completely changing the manager interface, which will break so many things.

Fortunately, the current mechanism mostly works with multiple managers and I just need to delete all vim.api.nvim_get_current_tabpage() to something like manager.tabid that is constant throughout the lifetime of the manager.
It is much harder to share the same state across multiple tabpages with one manager since for example when user :Neotree left on tab1, we have to spawn a tree on the left in tab2 as well with the same state but different window.

[cseickel]

Since the above code in lua means the exact same thing, it is very easy to make sources compatible with OOP without changing anything. The first notation makes it a bit easier to keep track of type annotations so I'd prefer the first style, but we can keep current sources mostly as-is thanks to the coincidence that most functions take state as the first argument.

That is a good point. Since classes in lua are really just hacky ways to fake an object, you are free to use either style and luckily it was written in a way that fits.

It is much harder to share the same state across multiple tabpages with one manager since for example when user :Neotree left on tab1, we have to spawn a tree on the left in tab2 as well with the same state but different window.

That is really weird to me. I think we should really flesh out (in a separate discusson) exactly what we want Neo-tree to do with regards to state that is accessible across tabs. How would we go about enabling this? Is this a configuration option, an additional position argument, an extra flag in the command?

Another option that would be very flexible and work well with edgy is to make the position options something that could be configured so that the user can define their own and specify where it wants to go and what the scope is. Something like:

positons = {
  left = { location: "left", scope: "tab" },
  current = { location: "current", scope: "window" },
  left_all_tabs =  { location: "left", scope: "global" },
  edgy_left = { location: "current", scope: "tab" },
  ...
}

[miversen33]

*Almost: user need to add approx 10-20 lines and implement cls.new that returns a state (self), which was done in sources/manager.lua > create_state before.

Just chiming in for clarity here. User here means a developer creating an external source yes? So myself and @ghostbuster91?

Because the alternative (user being an end user of Neo-tree), assuming those users will happily create 10-20 lines in their configuration is a very poor assumption that will absolutely lead to complaints/issues daily

[pysan3]

Yes, I meant developers here. Nothing so far needs to be changed for end users (I mean the config opts). Maybe some changes to the event handler but not sure now.

This is the current diff you'll need to change to buffers to make it compat with my current code, THAT IS STILL VERY WIP.

• setmetatable to neo-tree.sources.base (A new abstract class).
• M.__index = M
• Implement M.new that returns a instance of yourself.
• Instead of a callback style, the source has to call manager.done when rendering is done.
• And manager.fail if it fails.

One more, the manager won't require "neo-tree.sources.*.commands" and "components" but you have to set it to M.commands and M.components.

diff --git a/lua/neo-tree/sources/buffers/init.lua b/lua/neo-tree/sources/buffers/init.lua
index 94347c9..25ebb56 100644
--- a/lua/neo-tree/sources/buffers/init.lua
+++ b/lua/neo-tree/sources/buffers/init.lua
@@ -8,11 +8,42 @@ local items = require("neo-tree.sources.buffers.lib.items")
 local events = require("neo-tree.events")
 local manager = require("neo-tree.sources.manager")
 local git = require("neo-tree.git")
+local Path = require("pathlib")
 
-local M = {
+---@class NeotreeBuffers : NeotreeState
+---@field config NeotreeConfig.buffers
+---@field dir PathlibPath
+local M = setmetatable({
+  -- Attributes defined her end
   name = "buffers",
   display_name = " 󰈚 Buffers ",
-}
+  commands = require("neo-tree.sources.buffer.commands"),
+  window = {},
+  components = require("neo-tree.sources.buffer.components"),
+  renderers = {},
+}, {
+  __index = require("neo-tree.sources.base"), -- Inherit from base class.
+})
+M.__index = M
+
+---Create new manager instance or return cache if already created.
+---@param config NeotreeConfig.buffers
+---@param id string # id of this state passed from `self.setup`.
+---@param dir string|nil
+function M.new(config, id, dir)
+  local self = setmetatable({
+    id = id,
+    dir = dir and Path.new(dir) or Path.cwd(),
+    config = config,
+  }, M)
+  if not self.dir:is_dir(true) then
+    require("neo-tree.log").error("(%s) is not a directory. Abort.", self.dir:tostring())
+    return
+  end
+  return self
+end
 
 local wrap = function(func)
   return utils.wrap(func, M.name)
@@ -81,30 +112,20 @@ M.buffers_changed = function()
 end
 
 ---Navigate to the given path.
----@param path string Path to navigate to. If empty, will navigate to the cwd.
-M.navigate = function(state, path, path_to_reveal, callback, async)
+---@param path PathlibPath|nil Path to navigate to. If empty, will navigate to the cwd.
+M.navigate = function(state, path, path_to_reveal, window_width, manager, failed_args)
   state.dirty = false
-  local path_changed = false
-  if path == nil then
-    path = vim.fn.getcwd()
-  end
-  if path ~= state.path then
-    state.path = path
-    path_changed = true
+  if path and path:absolute() ~= state.dir then
+    local new_id = state.name .. state.dir:tostring()
+    local reason = string.format("dir %s is not supported.", path) -- If new_id is nil, this reason will be reported to the user.
+    return manager:fail(reason, new_id, state, path, path_to_reveal, window_width, failed_args)
   end
   if path_to_reveal then
     renderer.position.set(state, path_to_reveal)
   end
 
   items.get_opened_buffers(state)
-
-  if path_changed and state.bind_to_cwd then
-    vim.api.nvim_command("tcd " .. path)
-  end
-
-  if type(callback) == "function" then
-    vim.schedule(callback)
-  end
+  manager:done(state, nil)
 end
 
 ---Configures the plugin, should be called before the plugin is used.

[pysan3]

I'll definitely write a migration manuals especially for developers so please read that when it's ready.

[cseickel]

A lot of good points have been made here, let me address some of them in no particular order:

Do we need a Rewrite?

I agree that I'd rather not have any breaking changes for our consumers (end users and distro maintainers), but I think it's ok to make a few changes that will affect external sources so long as it is done long with the external source maintainers.

Also, i just want to clarify that I really meant that we should take the time to think about what is the ideal from a blank slate rather than "how can I patch what we have to fix one pain point at a time". That doesn't mean we ultimately re-write it but I find that leads to a better solution if you think it through that way. In the end you find a way to get from your blank slate ideas to a realistic amount of change.

The many faces of config

I put the config on the state because I wanted to have a system where there was one argument passed around to all functions that would contain ALL state (and I am thinking of config as state here) to make the functions simpler. That state contains the state of the source plus the global config.

Anywhere you see require("neo-tree").config is probably an instance where that state object is not available for some reason and is unusual.

I can see an argument to be made for always requiring config to be looked up instead of including it in the state object. That would be a significant breaking change though so it's probably not worth changing.

From the point of view of adding types, the state object will be a problem because it is different for each source. That would be an area where I'd inherit the config portions and leave the rest of it dynamic.

Callback hell

I honestly don't see the callback hell. There's not that much asynchronous code, in in many cases the same code can be synchronous or asynchronous and providing or omitting the optional callback is the difference.

This is a case here I think we would need some examples of what is wrong now, vs how it could be made better with nio, and how tha is better than plenary.

[pysan3]

Could you number your sections (1. Do we need a rewrite) so I can reference them easier?

Looking at the code, I don't think the code is much broken, but it is too long.
I really wonder why neo-tree/setup/init.lua is more that 700 loc, which should be just parsing the user config.
I don't think most of the code belong here, and I don't like the functions changing value of variables that are out of the scope of the function.

neo-tree.nvim/lua/neo-tree/setup/init.lua

Line 151 in c2a9e81

local last_buffer_enter_filetype = nil

Besides, I think there are many repetitive code thruout the codebase. So I think we need to reorganize and factor out similar codes to one place and decrease the code complexity.

Eg these look very similar, but are written in a very different location.

• neo-tree.nvim/lua/neo-tree/git/status.lua

  Line 9 in c2a9e81

  local function get_simple_git_status_code(status)

• neo-tree.nvim/lua/neo-tree/sources/common/components.lua

  Line 186 in c2a9e81

  if git_status:match("?$") then

I put the config on the state because I wanted to have a system where there was one argument passed around to all functions that would contain ALL state (and I am thinking of config as state here) to make the functions simpler. That state contains the state of the source plus the global config.

I might think this is a bad decision. Config should be a static value, which does not change regardless of the state of anything (remain constant). I understand that it'd be easier to pass around a table that contains everything, but I believe we should distinguish what should be reference only, and will be changed based on the state.

Anywhere you see require("neo-tree").config is probably an instance where that state object is not available for some reason and is unusual.

I see this referenced 29 times in the codebase, which I think is more than unusual and a couple more with utils.get_value. (8 times inside selector.lua, which I do see the reason tho)
I'm actually more into this way of referencing the config as it self documents that the values can only be referenced.

From the point of view of adding types, the state object will be a problem because it is different for each source. That would be an area where I'd inherit the config portions and leave the rest of it dynamic.

Lua_ls type annotation can inherit other classes so we can.

---@class NeotreeState
---@field x

---@class NeotreeFilesystemState : NeotreeState
---@field y
---@field specific_attributes

---@type NeotreeFilesystemState
local state = {}
state.x -- ok

I honestly don't see the callback hell. There's not that much asynchronous code, in in many cases the same code can be synchronous or asynchronous and providing or omitting the optional callback is the difference.

This is a case here I think we would need some examples of what is wrong now, vs how it could be made better with nio, and how tha is better than plenary.

I think I should rewrite one of the source with nio to give an example. I'll start doing this later down the list anyways, so I'll talk about this again then.

[pysan3]

After I'm done with type annotations, it might be better for me to implement a POC for one source with a huge refactor / rewrite (haven't decided yet) to show my ideal code.

[ghostbuster91]

fwiw I agree with @pysan3 that there is some kind of callback hell in the code that makes it hard to reason about the execution flow. Structured concurrency ftw. IMO all functions should be async (if possible) and they should be converted to regular functions only at the edges of the plugin.

Good example of the problem is how the filesystem source integrates with git.

[cseickel]

I do always start new projects with whatever the async flow is for that language so I would agree that this is better than callbacks. Over the course of this discussion I've come around to the idea that it would be worth the effort to make the change.

[cseickel]

I really wonder why neo-tree/setup/init.lua is more that 700 loc, which should be just parsing the user config.

Eg these look very similar, but are written in a very different location.

neo-tree.nvim/lua/neo-tree/git/status.lua

Line 9 in c2a9e81

local function get_simple_git_status_code(status)
neo-tree.nvim/lua/neo-tree/sources/common/components.lua

Line 186 in c2a9e81

if git_status:match("?$") then

I don't think those are similar at all. The first one parses the git status from the git binary and forces it into a simpler representation. The second line reads that simplified representation an decides what icons/highlight to display. One thing that can change there is that we parse git into a table that is more useful than just a string so there's less parsing in the display stage.
#1031

I might think this is a bad decision. Config should be a static value, which does not change regardless of the state of anything (remain constant). I understand that it'd be easier to pass around a table that contains everything, but I believe we should distinguish what should be reference only, and will be changed based on the state.

I agree. It is a huge breaking change though so the config on state will need to be deprecated for a major release before it is removed. Internally we can ignore the config on state and also not bother to document the type of config being there to inhibit new users from referencing it.

I think I should rewrite one of the source with nio to give an example. I'll start doing this later down the list anyways, so I'll talk about this again then.

I'm pretty sure that the filesystem is the only one that has any async at all. There are two paths that you might be able to separate and only do one: the file system scans and the git_status lookup.

[ghostbuster91]

Config should be a static value, which does not change regardless of the state of anything (remain constant)

I also wholeheartedly agree with this one.

It is a huge breaking change though so the config on state will need to be deprecated for a major release before it is removed.

What is the approach for introducing breaking changes in this project? Is it ok to introduce breaking changes across major versions? Do they have to be deprecated first? Does the deprecation need to have its own major release?

I'm pretty sure that the filesystem is the only one that has any async at all.

From the builtin ones - yes. But in general sources will be often async and the core should have first-class support for them.

[cseickel]

What is the approach for introducing breaking changes in this project? Is it ok to introduce breaking changes across major versions? Do they have to be deprecated first? Does the deprecation need to have its own major release?

It depends on the scope of the change and how many people it would affect.

When we replaced the old user commands with a single command that takes arguments, we left the old commands deprecated for an entire major release with a global variable to disable them which was the recommended config. The same is true for any changes to API's that are recommended in the documentation or frequently suggested in discussions / wiki.

Smaller changes where config variables are renamed or moved are automatically fixed for the user with a warning and can happen at any time.

In between those would be a judgement call based on how many people are affected and how much trouble it is for them to make the change.

The one hard rule is that someone's config can't break because of a change we have made within a major version.

[ghostbuster91]

regarding the rewrite, I agree to some degree with @cseickel

Also, i just want to clarify that I really meant that we should take the time to think about what is the ideal from a blank slate rather than "how can I patch what we have to fix one pain point at a time". That doesn't mean we ultimately re-write it but I find that leads to a better solution if you think it through that way. In the end you find a way to get from your blank slate ideas to a realistic amount of change.

I think that the decision whether to rewrite from scratch or do it in an incremental fashion depends mostly of the amount of changes that we want to do.

@pysan3 already gathered some major pain points that should be addressed. We should also think what works good in the current implementation.

Once we will have such a list with both pros and cons of the current codebase we can asses which way is the optimal one.

[pysan3]

How should we work with named states and share states across tabpages?

Conversation starts from here: #1340 (reply in thread)
@cseickel

1.

That is really weird to me. I think we should really flesh out (in a separate discusson) exactly what we want Neo-tree to do with regards to state that is accessible across tabs. How would we go about enabling this? Is this a configuration option, an additional position argument, an extra flag in the command?

From what I was imagining, we would have a config option config.share_state_among_tabs (boolean), which switches between a) completely separated states between tabpages, b) states synced for all tabs.

For the case of a, when user calls :Neotree left on tab1, nothing will happen in tab2, and when user calls :Neotree left in tab2 and expand ./foo/, tree in tab1 will not change at all. Same for named tabs; user can open states with same name on all tabs, and those states are complete different things (this is debatable[1] tho; should named states be shared across tabs regardless?).

For the case of b, when user calls :Neotree left, a window opens in tab1, and also in tab2 (and all other tabs). When user expands ./foo/ in tab1, the exact same thing will be shared across all tabs. In short, the same buffer is shown on all tabs (it must be different winids cuz windows cannot be shared across tabs). One thing to note is that neo-tree will try to copy the layout on au TabNew / TabNewEntered.

If this is a good idea, I've implemented probly like 60% of the code now? So I'd like to share the code when I'm done and ask for help testing and giving feedbacks about the new mechanism. (btw I can easily switch the behavior of [1] so both are ok).

Sharing the state across tabs is not the difficult part, but sharing the position and syncing toggle (mount/unmount window) of that state across tabs is the tricky part since windows cannot be shared across tabs.

2.

With my current implementation, when user :Neotree filesystem left and then :Neotree filesystem right, the window on the left disappears and moves to the right. However, with :Neotree filesystem left | :Neotree buffers right, both windows stay. This is to make it possible to show two filesystems at once by naming both states with :Neotree filesystem left dir=./foo id=FooDir | :Neotree filesystem left dir=./bar id=BarDir.

I think this is a nice usage of named states.

And we can implement copy files from one dir to a whole another dir as a windfall with neo-tree ;)

What do you think about this?

---

3.

Another option that would be very flexible and work well with edgy is to make the position options something that could be configured so that the user can define their own and specify where it wants to go and what the scope is. Something like:

positons = {
  left = { location: "left", scope: "tab" },
  current = { location: "current", scope: "window" },
  left_all_tabs =  { location: "left", scope: "global" },
  edgy_left = { location: "current", scope: "tab" },
  ...
}

I think this can mostly be achieved with named session but I'm not exactly sure. But I must say that if we allow both tab shared and non-shared states, the code will be pretty complex.
This means that the scope will change dynamically with named states. If user calls :Neotree id=Foo filesystem left scope=tab and then :Neotree id=Foo scope=global, the expected behavior would be to elevate the scope of the state and start syncing the the position across tabs, which will be a hassle.

[pysan3]

One more question, for the case of 10, 11 above, what should I do when the size (height, width) of the windows are different?

If we set the exact same buffer, the content in one of the windows will be broken.

[pysan3]

The system I was proposing would not have an id= parameter at all. The position name is the only identifier exposed to the user. Internally, the manager will use that position name as the id within a scope. The idea is that the user is predefining IDs in the form of this position config, rather than creating new combinations on the fly at runtime.

If this is the case, :Neotree left(-tab) and :Neotree right(-tab) will be different states, while current neo-tree behavior will close left and then copy the tree (including the expand-folder states) into the right pane.

If you say we should instead assign all left,right,top,bottom into the same ID (like split-tab), then I think it breaks the trick used in edgy.

Also, current code of neo-tree also syncs the state with float, so it's getting more complex.

[cseickel]

One more question, for the case of 10, 11 above, what should I do when the size (height, width) of the windows are different?

☹️

If this is the case, :Neotree left(-tab) and :Neotree right(-tab) will be different states, while current neo-tree behavior will close left and then copy the tree (including the expand-folder states) into the right pane.

☹️ I suppose that is a big breaking change. I don't know if people generally liked that behavior or not.

If you say we should instead assign all left,right,top,bottom into the same ID (like split-tab),

I didn't say that at all. left, right, top, bottom would each be different ids within scope "tab".

Either way, I now realize that my idea does not really work as it is too big of a change from how it works now. Let's drop it and explore your idea instead. I know you posted a link to a WIP, I'll take a look tomorrow.

[pysan3]

Seems like the sneak peak can only be played on chromium?

https://github.com/nvim-neo-tree/neo-tree.nvim/assets/41065736/c9179ea1-d43c-4ea2-ba70-3f74cdb8ba79

Here's the link if you can wget it. Rename it to mp4 and you should be able to play it.

[pysan3]

Update:

I've managed to implement it with much extensibility and now user can change the scope on the fly as well. We could (and maybe should imo) disable this in the command args parser level to make the UI simple, but the manager API is very well capable of handling that.

1. :Neotree filesystem = :Neotree source=filesystem position=left scope=tab id=filesystem__internal_<tabid>
2. :Neotree filesystem current = :Neotree source=filesystem position=current scope=window id=filesystem__internal_<RANDOM_BYTES> (so we can have as many window-scoped currents as we want, and this is a different state from 1.)

And the manager is cleaver enough to overwrite current window when :Neotree buffers current but not when :Neotree filesystem current again. However, when user overwrites with buffers source, calling filesystem current again will return a new state, and the old state is already nil-ed.

This is not the case if :Neotree filesystem current scope=tab. This will in fact show the same thing from 1. and rerender it to fit the current window after closing the left window.

When user :Neotree left, then it will first close the tree rendered in current window, bring back the old buffer, then create a new split on the left, and rerender the tree to fit the left split. During all of this, state.bufnr should always be the same. (..theoretically, I'm not 100% sure. Haven't tested it thoroughly yet.)

[pysan3]

@cseickel Do you like this?

It's auto generated via this action and it uses sphinx under the hood.
And you've got many other themes to choose from: https://sphinx-themes.org/

BTW you also get a search page ;)

[cseickel]

I like the site and the library. Are you suggesting that we can generate docs like this for neotree?

[pysan3]

Glad to hear that ;)

Yep, all you need is the docstrings for each function and some cryptic files;

• index.rst: https://github.com/pysan3/pathlib.nvim/blob/main/index.rst (landing page)
• Pathlib.rst: https://github.com/pysan3/pathlib.nvim/blob/main/doc/PathlibPath.rst
  • these filenames appear on the top bar.

Of course you can go full into this and write a full document manually in rst file, but nah, just let it autogenerate lol.
And except for index.rst, all other pages can be written in markdown. So this page is generated from README.md.

[pysan3]

I've added a section to introduce nvim-nio and explain how to use it.
cc @cseickel

https://github.com/pysan3/neo-tree.nvim/blob/v4-dev/MIGRATION-v4.md#nio-async-vs-callbacks

Hope this motivates you more towards the migration ;)

[cseickel]

Nice explanation. I definitely think integrating nio will be a big improvment.

[pysan3]

Hey @cseickel

While I was rewriting commands.lua, I have a question.

neo-tree.nvim/lua/neo-tree/sources/filesystem/lib/fs_actions.lua

Line 421 in 7d3b020

M.delete_node = function(path, callback, noconfirm)

A) Inside delete_node, it says Could not read file/dir: ..., attempting to delete anyway... but is returning right after that, meaning that it is actually NOT attempting to delete anyway. TBH I think we don't need to try to delete it and I agree with the code and not to the comment.
Can I change the behavior to abort when target not found (with an error message)?

B) Right above that, when the filetype is a link, the _type is substituted with the realpath's fs_type. After that, inside the do_delete function, on line 492, when _type == "directory", the code tries to use the shell command to delete the directory. However, when the file path is a link, rm -Rf ./hyperlink will NOT delete the destination but just the link itself (same for Windows).
Although the code is intended to remove the target dir, I think delete_node on a symlink should just remove the link file, so using vim.loop.fs_unlink is correct here, regardless of the type of the destination. WDYT?

[cseickel]

A) Inside delete_node, it says Could not read file/dir: ..., attempting to delete anyway... but is returning right after that, meaning that it is actually NOT attempting to delete anyway. TBH I think we don't need to try to delete it and I agree with the code and not to the comment.
Can I change the behavior to abort when target not found (with an error message)?

Definitely, we can ignore the comment.

B) Right above that, when the filetype is a link, the _type is substituted with the realpath's fs_type. After that, inside the do_delete function, on line 492, when _type == "directory", the code tries to use the shell command to delete the directory. However, when the file path is a link, rm -Rf ./hyperlink will NOT delete the destination but just the link itself (same for Windows).
Although the code is intended to remove the target dir, I think delete_node on a symlink should just remove the link file, so using vim.loop.fs_unlink is correct here, regardless of the type of the destination. WDYT?

Deleting a symlink should remove the link itself and not affect the target directory/file is is linked to.

[pysan3]

B) Right above that, when the filetype is a link, the _type is substituted with the realpath's fs_type. After that, inside the do_delete function, on line 492, when _type == "directory", the code tries to use the shell command to delete the directory. However, when the file path is a link, rm -Rf ./hyperlink will NOT delete the destination but just the link itself (same for Windows).
Although the code is intended to remove the target dir, I think delete_node on a symlink should just remove the link file, so using vim.loop.fs_unlink is correct here, regardless of the type of the destination. WDYT?

Deleting a symlink should remove the link itself and not affect the target directory/file is is linked to.

Yes but when symlink points to a directory, your code tries to rm -rf symlink, where I believe it should do vim.loop.fs_unlink(symlink) instead.

[cseickel]

B) Right above that, when the filetype is a link, the _type is substituted with the realpath's fs_type. After that, inside the do_delete function, on line 492, when _type == "directory", the code tries to use the shell command to delete the directory. However, when the file path is a link, rm -Rf ./hyperlink will NOT delete the destination but just the link itself (same for Windows).
Although the code is intended to remove the target dir, I think delete_node on a symlink should just remove the link file, so using vim.loop.fs_unlink is correct here, regardless of the type of the destination. WDYT?

Deleting a symlink should remove the link itself and not affect the target directory/file is is linked to.

Yes but when symlink points to a directory, your code tries to rm -rf symlink, where I believe it should do vim.loop.fs_unlink(symlink) instead.

I was not commenting on whether the code is correct or not, I was confirming what the expected result is. If rm -rf symlink removes the linked to directory, then that is wrong.

[pysan3]

I see. Good to hear that. I'll fix it in the rewrite.

[pysan3]

@cseickel What is the difference between move command and rename command?

[pysan3]

Rename is a simpler form of move that only moves within the same directory.

Do I need to restrict this?
Like with the current codebase, user could pass folder/filename.txt to rename and it works when folder/ already exists.

[cseickel]

I would just keep the existing behavior because someone out there depends on it.

[pysan3]

Then wouldn't rename and move basically be the same thing, but just move to child folders? I don't see a reason to have this restriction.

What do you thing if I just...

M.rename = function(state)
  return M.move(state)
end

Pathlib automatically works with ../ so we now have additive functionality btw.

[cseickel]

It seems perfectly reasonable to have rename just be an alias for move, tbh. I have never used the move command myself so I can't honestly say if there are different semantics between them. I always use cut/paste to move a file personally.

[pysan3]

I always use cut/paste to move a file personally.

Literally me lol.

OK, I'll alias it to move for simplicity.

[pysan3]

@cseickel

I think I'm ready for the first alpha test! It's still way away from official release, and lacking a lot of functionalities.

With this first implementation, I mostly focused on speed and async-ability (does not block neovim's main runtime thread). For example even when using the tree with the linux kernel (or any gigantic project), you should not feel that much lag when opening the tree.

I also made it so that it can do incremental update of the tree. Eg 1. git status will be lazily evaluated, and 2. When you for example toggle a large directory, the rendering will lag behind but user can interact with the tree as they want; user can toggle a different dir as well, and both will be opened when rendering is done.
This is close to debounce_strategy.CALL_LAST_ONLY but should be much more reliable and fluent thanks to nio.task.cancel().

Could you test the code and poke around to see if the performance is good on your device?
I'm pretty sure there will be a lot of errors and no need to dig into the code, but I want to hear your first impressions.

Install

I will recommend you read https://github.com/pysan3/neo-tree.nvim/blob/v4-dev/MIGRATION-v4.md#setup-lazy-for-devs to have v3 neo-tree and my dev code side-by-side, but this is an easy copy paste instead if you don't care.

return {
  "pysan3/neo-tree.nvim",
  branch = "v4-dev",
  version = false,
  dependencies = {
    { "nvim-lua/plenary.nvim" },
    { "MunifTanjim/nui.nvim" },
    { "3rd/image.nvim" },
    { "pysan3/pathlib.nvim" },
    { "nvim-neotest/nvim-nio" },
    { "nvim-tree/nvim-web-devicons" },
  },
  opts = {
    -- ...
  }
}

Things that are not implemented yet

• Any sources. We now only have one source: filetree, which is a replacement for filesystem. The setup function will limit to only this source, and will overwrite whatever you passed to config.sources for now.
• source selector
• preview window
• help window
• order_by_* (o key)
• event handlers (some work, some don't)
• anything related to changing cwd

Node Sorting

Nodes are only sorted alphabetically now. No config options are respected.

Search

I'd like to reimplement this so all sources will automatically have searching ability without any additional code to each sources.
A base class (which all sources will inherit) will prepare everything needed for searching.

Command Completion

No auto completion after the :Neotree command yet.

---

For more details please read https://github.com/pysan3/neo-tree.nvim/blob/v4-dev/MIGRATION-v4.md#road-map.

cc
@miversen33
@nhat-vo
@ghostbuster91
@mrbjarksen

[ghostbuster91]

Hi, I just tried to run it and it failed with a following error:

   Error  08:09:52 AM msg_show.lua_error   Neotree  Error executing vim.schedule lua callback: ...ackages/start/p_nvim-neotree/lua/neo-tree/manager/wm.lua:32: attempt to call field 'ge' (a nil value)
stack traceback:
	...ackages/start/p_nvim-neotree/lua/neo-tree/manager/wm.lua:32: in function 'get_highlight_string'
	...ackages/start/p_nvim-neotree/lua/neo-tree/manager/wm.lua:93: in function 'create_win'
	...kages/start/p_nvim-neotree/lua/neo-tree/manager/init.lua:541: in function 'create_win'
	...kages/start/p_nvim-neotree/lua/neo-tree/manager/init.lua:228: in function 'open_state'
	...kages/start/p_nvim-neotree/lua/neo-tree/manager/init.lua:215: in function 'navigate'
	...kages/start/p_nvim-neotree/lua/neo-tree/command/init.lua:66: in function <...kages/start/p_nvim-neotree/lua/neo-tree/command/init.lua:65>

Do I need have newer version of neovim?

NVIM v0.9.5
Build type: Release
LuaJIT 2.1.1693350652

   system vimrc file: "$VIM/sysinit.vim"
  fall-back for $VIM: "
/nix/store/23y9a26pwxx7l5a3c3ph64bhamfpp6zm-neovim-unwrapped-0.9.5/share/nvim
"

Ironically it fails on this line:

 if vim.version and vim.version.ge(vim.version(), { 0, 7, 0 }) then

[pysan3]

Loooooool so ironic 😂

That's an easy fix. I'll ping you when I can reach my pc. Besides that could you also test the search feature as well when it's ready?

#1340 (reply in thread)

[pysan3]

@ghostbuster91 Should be fixed! Thanks in advance for testing :)

[ghostbuster91]

I confirm, it works now, thanks for the quick fix 👍 I would be happy to switch to it as my daily driver to test it more intensively but I would first need to have 2 features: graying out ignored directories/files and ordering of files directories. I can implement those myself if you think it's a good idea and don't see any major obstacles. Let me know.

[pysan3]

@ghostbuster91 Thanks a lot for testing.

It looks like I need to rewrite it quite a lot of components to make get git highlights. I'm working on it so please wait a bit.

Reordering of files should work with a recent update so could you check it out?
The help menu (you get by o or ?) should also work so could you check it out.

[pysan3]

@cseickel As winfixbuf is merged into neovim, we might want to completely rewrite the steal prevention mechanism.

https://www.reddit.com/r/neovim/s/i75dYg7gmT

It's seriously gonna be a pain in the * but we have to take into account that other plugins may also be using this option, meaning we can't rely on get_appropriate_window function actually being functional.

I'm very pessimistic that it'll work well with close_if_last_window.

[pysan3]

Sneak peek; search is gonna be buttery smooth + it will be integrated to all sources for free ;)

record.mp4

[pysan3]

@cseickel I added help menu and sorting (o: order_by_*).

Would you mind testing?

[cseickel]

You know, I just realized when testing this out that I mistakenly thought I was testing the last time when I was actually still using the main repo. The problem was that I had a lazy config that automatically found and used local repos.

Anyhow, I guess that's why I had other sources and the fuzzy sorter. Also, it means that the existing search is already extremely fast in all situations so there is really no room for improvement from a performance perspective (at least on my computer.)

As to the new feature of sorts, it seems to work fine except for the fact that I can't sort on Size because the size column is not populated.

[pysan3]

@cseickel Delete these lines and try again on a working git repo.

https://github.com/cseickel/dotfiles/blob/d08b0c37f9ba64a0edb830445b2d788da17953e4/config/nvim/lua/plugins/neo-tree.lua#L233-L240

Also the biggest difference is with the first search after you open neovim. From the second search, old code also caches some information to speed up tree rendering and there might not be much difference on a relatively small folder.

[cseickel]

Honestly, it seems like main is faster than this branch after commenting out my fd config. Also, there is a bug in your branch where the search does not reset when backspacing out the search term.

[pysan3]

@cseickel Please give me a screen record.

And please show me which commit you are testing against since I might review it later in the development.