feat: New node-scanning algorithm and empty folder states

[zenoli]

This PR implements open/closed states for empty folders which is required in order to complete my other PR.

This involved actively checking wheter newly revealed folder nodes are empty and if so, showing the "empty folder" icon directly. This led me to the idea to also check if the folder contains only a single child directory. By applying this check recursively, and scanning down the chain such directories, neo-tree will directly display all grouped directories if "group_empty_directories" is set. This recursive scanning will not recursively explode and scan exponentially many nodes, as it only scans what really needs to be shown in the tree. (See the GIF below).

This PR is still a WIP and before I proceed I would like to get some feedback on the following points:

1. Is this functionality appreciated?
2. As of now, I basically rewrote sync_scan and async_scan in fs_scan.lua. Would you want to make this the default behaviour or should it be opt-in setting a new config option (for example "prescan_empty_folders")?
3. In the original implementation of sync_scan and async_scan there is the possibility to scan recusively based on what is set in context-recursive. From what I could tell, recusive option only behaved correctly in sync version (and of course blocked neovim if opening neo-tree in large projects.) Is this code path actively used? If yes and in case my implementation would become the default, I would have to add this recursive behaviour to the new implementation as well.

Best,
Zenoli

[cseickel]

• Is this functionality appreciated?

Yes! I intended for it to work this way originally, but it was problematic so I called it good enough to do one level at a time.

• As of now, I basically rewrote sync_scan and async_scan in fs_scan.lua. Would you want to make this the default behaviour or should it be opt-in setting a new config option (for example "prescan_empty_folders")?

It should be the default, only if the user chooses to group empty directories.

• In the original implementation of sync_scan and async_scan there is the possibility to scan recusively based on what is set in context-recursive. From what I could tell, recusive option only behaved correctly in sync version (and of course blocked neovim if opening neo-tree in large projects.) Is this code path actively used? If yes and in case my implementation would become the default, I would have to add this recursive behaviour to the new implementation as well.

It is only used by the expand_all_nodes command, which does get run with the sync version only:

neo-tree.nvim/lua/neo-tree/sources/filesystem/commands.lua

Line 72 in 5c209e5

fs.toggle_directory(_state, node, nil, true, true)

[zenoli]

Ok, then I suggest to do the following:
Make the new "pre-scanning" algorithm the default. If "group_empty_directories" is not enabled, it will still work as expected, we merely already loaded potential single-sub directories with negligible performance overhead. This would make the code a bit simpler and easier to read.
About the recursive option: I tested the expand_all_nodes function with the new algorithm and it works as expected (the new algorithm does not need a recursive flag. About the performance of this operation: It takes some time on a large project but I experienced the same with the "old/original" version and I think this is just intrinsic to the problem. Also, after expanding all nodes once, everything is cached and subsequent calls are very responsive (which I think was the same in the original version).

Some ui tests fail, because the requirements changed with the new functionality added. As we decided to go with the new approach I would need to adapt those tests accordingly.

If you give me the "GO" for these suggestions to be implemented, I'll get it done :-)

[cseickel]

I've learned the hard way that there are always some users out there that have projects that are so different from mine that there will be a large performance impact to them from everything extra I do. We really do need to make this an opt in feature. Even if I did intend for it to be default, I would still provide an option for people to go back to the old behavior just in case. This is a very delicate area.

Also, if the expand_all_nodes works properly, then that would mean you are actually recursively visiting the entire tree, which would have a huge performance impact. We absolutely cannot do that every time. (I haven't read the code thoroughly enough to see if this is the case.)

I just noticed that you went back to using plenary's scandir functions, I actually switched off of them for good reasons and I would not want to go back to using them. Basically, they are slower and the hidden files/gitignored logic is wrong.

[zenoli]

I've learned the hard way that there are always some users out there that have projects that are so different from mine that there will be a large performance impact to them from everything extra I do. We really do need to make this an opt in feature. Even if I did intend for it to be default, I would still provide an option for people to go back to the old behavior just in case. This is a very delicate area.

Ok, then I will make everything newly added "opt-in".

Also, if the expand_all_nodes works properly, then that would mean you are actually recursively visiting the entire tree, which would have a huge performance impact. We absolutely cannot do that every time. (I haven't read the code thoroughly enough to see if this is the case.)

Yes I think when using my algorithm it recursively visits the entire tree and it is laggy doing it initially on a large project.
But isn't that the idea behind "expand_all_nodes"? Or what does this do in the current state of the plugin?

I just noticed that you went back to using plenary's scandir functions, I actually switched off of them for good reasons and I would not want to go back to using them. Basically, they are slower and the hidden files/gitignored logic is wrong.

Oh boy. I just added this because I saw it being used somewhere in the codebase already and hence thought this was ok to use. So I should only use the built-in uv I/O functions then? Is it ok to use plenarys async module to avoid callback hell?

[cseickel]

Yes I think when using my algorithm it recursively visits the entire tree and it is laggy doing it initially on a large project.
But isn't that the idea behind "expand_all_nodes"? Or what does this do in the current state of the plugin?

expand_all_nodes is an optional command that is not mapped by default. When executed, it will re-scan recursively. There is no setting to scan recursively on a normal load though.

I just noticed that you went back to using plenary's scandir functions, I actually switched off of them for good reasons and I would not want to go back to using them. Basically, they are slower and the hidden files/gitignored logic is wrong.

Oh boy. I just added this because I saw it being used somewhere in the codebase already and hence thought this was ok to use. So I should only use the built-in uv I/O functions then? Is it ok to use plenarys async module to avoid callback hell?

Plenary in general is a good library and especially the async module is very useful, I just don't like the scandir functionality. It's good for quick and easy, but when scanning the filesystem is the primary thing you do, you really need to go to a lower level to get all the functionality and speed correct.

[zenoli]

@cseickel hey, I implemented the changes.
All new functionality is now behind a new config option scan_mode. The default value shallow refers to the current behaviour: Don't look into directories to detect whehter they are empty. The value deep refers to the newly implemented pre-scanning algorithm which directly displays empty folders (or grouped empty directories if they are enabled) when such a folder is currently visible in the tree.
I also added a new folder icon option folder_empty_open which defaults to the same symbol as the default folder_empty. However, this is currently only used when using scan_mode = "deep".
Maybe I could add a separate config option enable_open_empty_folders but I did not want to put too many new config options.

Let me know what you think. If you have better name suggestions, I am more than happy to hear them :-)

[cseickel]

Hey @zenoli, I know this is annoying but I just merged another PR that affects the directory scanning and causes conflicts with yours: #618

The reason I put that new one first is because it was really a bug, it's relatively simple, and it impacts most of the user base. Hopefully it's not too much trouble for you to incorporate those fixes.

---

I scanned your code and this is my understanding, correct me if I am wrong: If the option is on, it will always recurse 1 extra level, but then will stop and throw away the results if the next level is not an empty folder. Is that accurate?

[zenoli]

Hey @cseickel I just saw the conflicts, but I don't think it will be much of an effort to incorporate them, no worries.

I scanned your code and this is my understanding, correct me if I am wrong: If the option is on, it will always recurse 1 extra level, but then will stop and throw away the results if the next level is not an empty folder. Is that accurate?

Maybe let me explain it in an example just to be concrete:
Let's say I have the following directory structure:

parent/
  empty/
  full/
    somefile.txt
  d1/
    d2/

Initially parent/ is collapsed. Now, if I expand parent/ in the "old/current" behaviour only parent/ gets scanned. We inspect all of it's children and check if they are directories (in which case the folder_closed icon is shown) or if they are files.

However, what I try to achieve is to have more information after expanding a directory. I also want to know which if the children are empty, full, or containing a chain of single directories (to display them all on one line if group_empty_directories is enabled.)
To do this I - as you wrote above - also scan the children (i.e. d1, empty, full)

• When scanning empty I can see that the directory is empty and display the folder_empty icon.
• When scanning full I see that it contains files and display the folder_closed icon.
• When scanning d1 I see that there is exactly one child which is also a directory and I recursively scan this child.

I hope this makes things clearer.

[cseickel]

I hope this makes things clearer.

Yes, that sounds perfect. Thanks for the walkthrough.

[zenoli]

Hi @cseickel,
It's been some time. Will you have time to review this? Or did you expect me to first squash the commits? (I wanted to do this after it is ready to be merged by I'm not sure what your policies are).

[cseickel]

It's been some time. Will you have time to review this? Or did you expect me to first squash the commits?

I was actually waiting for you to rebase and fix the merge conflicts in fs_scan.lua first.

[zenoli]

@cseickel I used to rebase a while ago, but since then new conflicts arose which I didn't notice :-)
I rebased again!

[cseickel]

Thanks @zenoli, I'll take look at this tonight. I promise I'll merge or reply to this before merging any other changes.

[zenoli]

Perfect! In case you have time, maybe also look at #522 which is already rebased on top of this branch.

[cseickel]

Looks good!