Single NeoTree command to interface with different sources

[levouh]

Still super work-in-progress, but I wanted to get feedback early before spending a bunch of time doing something that ultimately needs to be changed. The current status:

1. NeoTree command works for all previous commands (I think, I have not used most of them much nor do they have tests so it was all done manually)
2. make test can be used to run tests using plenarys busted module
3. Some refactoring of the documentation (mostly just the .md files for now, will need to be converted into the .txt docs for Vim :h support, or we can look at using md2vim so you only have to maintain a single one).

A brief description of the changes here:

1. A utils/command module that handles all interfacing with the commands, including parsing, validating, and running the appropriately associated method
2. A CommandParser object, which I do not think is super necessary as we are able to unpack({<f-args>}) in the command. This function basically just ends up calling vim.split on each of the value in the table, so it comes across as quite overkill, however it does make testing the parsing of commands easier to have it separated this way.
3. A Action object, which mostly handles taking the parsed arguments for the command and determining what action needs to be called, and with what arguments. The abstraction layer here isn't really necessary pending some of the middle layers that exist (discussed below). Being able to keep a (again abstracted/separate for easier testing) mapping here for a function and extra arguments that need to be passed to it makes it easier to use the same underlying functions in neo-tree.lua but pass them different arguments. Separate functions in that same module (neo-tree.lua) with some hard-coded arguments to call other functions, so like:

M.newaction = function(...)
  M.otheraction(..., true, true)
end

does the same thing and might end up being a bit easier. Either way, having some "layer" like this seems necessary for all of the variations of the commands.

---

Again, I am posting this now for some early feedback and it is not complete yet, however I think it is still a bit jank the way that the command interfaces with the different methods in the main neo-tree.lua file. In a perfect world we might be able to pull sources into their own objects that all obey a contract, then this way the functions being called on them are "guaranteed" to work.

I'd like to mess with it a bit more (as well as talk here) to see what that layer might look like, both in the short term (say with no refactoring) and in the longer term (perhaps some refactoring). A good example for "user-implemented sources" would be how nvim-cmp does it in my opinion.

---

Some questions too:

1. It seems like having an alias for the current source (e.g. focused or perhaps last used?) might be useful so that users do not have to define commands over and over again for each different source. Something like all might also be useful, but I'm not sure what the use case for having multiple trees open is. Thoughts?

[cseickel]

Thanks @levouh , I'll take a look. As for the question about having multiple window open, I do see a use case for that. Some people, even myself, may want to have trees open on both the left and the right. Say, a file tree on the left and a symbol tree on the right. The more sources we have the more likely this feature will be used.

Also, we definitely do need some kind of interface, base class, or similar concept for sources. I'm new to lua and don't quite grok the class system yet. I'll take a look at nvim-cmp top see what they have done.

[cseickel]

I have a couple of top level comments:

1. This is all excellent work, I'm really grateful to have your contributions here!
2. I have concerns about breaking changes. The top level methods are a bit of mess because I kept just accommodating one more use case without breaking prior ones, hence the odd order and amount of options. I like what you have done, but we may need to make this a 2.x or otherwise give backwards compatibility some thought. Some people may be using those lua methods.
3. Likewise, I definitely don't want to remove the old commands right away. We can stop documenting them, or mention them as deprecated somewhere.
4. I think every option should have a default value, so the user can just issue NeoTree and have it work.
5. Having a "current" source doesn't make sense because there can be more than one, but there is already a "default_source" which defaults to "filesystem"
6. The window argument should be position, which acts as a temporary override to the value of window.position in the source's config. The valid values there are "top", "left", "right", "bottom", and "float". I don't think "top" and "bottom" make much sense for a tree, so I don't usually mention them, but technically they work.
7. We will need another action, which doesn't yet exist, which is like reveal but does not focus the window. I'm not sure what to call it yet. Maybe "locate"?
8. I notice in the plugins file your questions about NeoTreeShow, it works fine for me in main.

There have been a lot of changes merged since this branch. At least one of them was a fix for toggle commands. I also added a log module copied from plenary and modified slightly. Along with that log, I added two new (undocumented) commands:

NeoTreeSetLogLevel  <level>
NeoTreeLogs

These were really added for myself to use. Log levels below info only go to the file, which you have to enable in your config:

      require("neo-tree").setup({
        log_level = "debug",
        log_to_file = true,
      })

[cseickel]

@levouh I don't know if you've made any attempt to create a base class for sources, but I just put a little time into it. If you were going down that path too I think we should sync up.

It's a fair amount of work and I'm not sure if I am going to continue it as part of the feature I am working on now. Something definitely needs to happen because the common functionality between the sources is really growing. Now that I am adding events and file watchers I need to add a dispose method to clean this stuff up and that's the last straw. Here is what I have done so far on that: 8aa4526

It doesn't actually work yet.

[levouh]

Awesome! I have not done any work in that regard yet (wanted to get your general feedback on this PR first), but I'm honestly glad that you took a stab first as I am much less familiar with the code base and what it might need to look like. Happy to provide feedback as well if you'd like.

If you were going down that path too I think we should sync up

Either way I think this would be a good idea, however I imagine most of the classes will manifest themselves into a common set of actions rather naturally. As long as that is mostly the case (some common-ish contract that classes must follow), then the commands are super easy. Pending how aggressive you want to be on your side, it will likely be a good idea to wait to do this PR on top of your work.

[cseickel]

Agreed that the commands should come after the common contract.

I am starting to lean toward just using the "base source" as a module that the other sources deep copy and merge themselves onto as the means of inheritance rather than go all the way to classes. This is what i have done with components. The reason for this is that I don't actually want to encapsulate state, I just want to reuse and selectively override static methods. One advantage here is that it would not require any changes in the exported interface of sources. If you have any thoughts on this I would love to hear them.

[cseickel]

@levouh Can you let me know if you plan on continuing this work or not? Whether you'be just been busy or have lost interest, it's OK either way. I just want to know if I should continue it on my own.

Thanks!

[levouh]

I completely forgot this was in my court. Feel free to work on/finish it if you'd like, otherwise I can get back to it next week-ish @cseickel.

[cseickel]

I'm very happy to leave it with you if you are still interested @levouh, I'm sure you will do a good job with it! let me know if anything changes.

When you do get back to it, I had a thought about the command syntax. All of the arguments have unique values that don't conflict, so we can probably support space separated options in any order without issue, but leave the option=value syntax as optional for future disambiguation:

NeoTree reveal filesystem left
NeoTree filesystem reveal left
NeoTree focus left (filesystem implied as it's the default source)
NeoTree toggle show filesystem float
NeoTree float filesystem show toggle
NeoTree buffers right
NeoTree action=reveal position=left toggle=true source=buffers

[cseickel]

I'm going to close this PR because it seems dead. I've started a new branch for this and I will implement this feature. Thanks so much for the discussion and the portion you did complete @levouh! Even though I started fresh, I keep referring to the work here as a guide and it's been very helpful.

The new branch is at https://github.com/nvim-neo-tree/neo-tree.nvim/tree/feat/single-command-wth-args

[levouh]

No worries, absolutely understandable @cseickel ❤️