feat: Allow to expand until certain condition is met #2790

ghostbuster91 · 2024-06-01T13:14:24Z

This is my attempt to solve #2789

With this change I can now define the following mapping:

local function stop_expansion(_, node)
    return false, stop_expansion
end
local function expand_until_non_single(count, node)
    local cwd = node.link_to or node.absolute_path
    local handle = vim.loop.fs_scandir(cwd)
    if not handle then
        return false, stop_expansion
    end
    local status = git.load_project_status(cwd)
    populate_children(handle, cwd, node, status)
    local child_folder_only = explorer_node.has_one_child_folder(node) and node.nodes[1]
    if count > 1 and not child_folder_only then
        return true, stop_expansion
    elseif child_folder_only then
        return true, expand_until_non_single
    else
        return false, stop_expansion
    end
end
map('n', 'E', function()
    api.tree.expand_all(nil, expand_until_non_single)
end, opts("Expand until not single"))

This works as expected however it has some downsides:

the api is very flexible but at the same time quite complex. I had to do it in this way because the expand_all function will not expand the last directory. It stops as soon as should_expand returns false but I wanted the last directory to be expanded.
~~I had to copy populate_children method which is not a part of public api~~ replaced with explorer.explore(node, status)

Btw if this got accepted in some form I am going to replace the default expand behavior in my config with this, since I find it so useful.

alex-courtis · 2024-06-07T04:53:11Z

I love the concept and it is something we should do. Tried your example after a lot of hacking and it looks to work nicely.

Does nothing when called from the root node
Needs documentation

Use of private functions in your example aside, this is far too complex and not approachable for users. Having to populate the children is the key problem.

Suggestion: come up with a simple use case and example - one that would go in the documentation. We can start simplifying from there.

Perhaps I'm misunderstanding: why does this need to be an iterator? I was imagining the user would supply a simple boolean returning function to test against a node.

alex-courtis · 2024-08-25T02:52:20Z

Any updates on this one @ghostbuster91 ?

ghostbuster91 · 2024-09-01T15:07:07Z

Sorry, I was quite busy. I am going to take a look at it.

I love the concept and it is something we should do.

I am glad to hear that!

Perhaps I'm misunderstanding: why does this need to be an iterator? I was imagining the user would supply a simple boolean returning function to test against a node.

Because the simple fn: node => boolean is insufficient in my case. Take a look at the following example:

a
└── b
    └── c
        ├── c2
        │   └── d2 (file)
        └── d (file)

When expanding a I would like to expand b and c.
However c has multiple children and also directories inside so it wouldn't pass a simple should_expand function.

In my case the condition is following:
Keep expanding until you reach a node that has more than one children but expand this node as well. In other words the expansion is inclusive.

Perhaps this could be expressed in simpler way for example by using a flag - inclusive=true on the user side, however I thought that using this approach would be more elastic and elegant. Though I am not attach to it, so if you have an idea how to simplify it I will happily implement it.

come up with a simple use case and example - one that would go in the documentation.

update:

I would see it as:

local function expand_one_more(node)
   return {expand = true, fn_next = stop_expansion}
end

local function expand_until_non_single(node)
    local has_one_child_folder = explorer_node.has_one_child_folder(node)
    if has_one_child_folder then
        return {expand = true, fn_next = expand_until_non_single}
    else
        return {expand = false, fn_next = expand_one_more}
end

local f = function(node)
    if node.open then
        lib.expand_or_collapse(node, nil)
    else
        if node.nodes then
            api.tree.expand_all(node, expand_until_non_single)
        else
            edit("edit", node)
        end
    end
end

map('n', '<CR>', wrap_node(f), opts("Expand until not single or collapse"))

ghostbuster91 · 2024-09-01T20:16:13Z

In the end I was able to simply replace the iterator with accessing node's siblings via node.parent.nodes. With this the user side function for my particular use case looks as follow:

local function expand_until_non_single(count, node, populate_node)
    populate_node(node)
    if node.nodes == nil or not node.parent.open then
        return false
    end
    local has_one_child_folder = explorer_node.has_one_child_folder(node)
    local has_no_siblings = explorer_node.has_one_child_folder(node.parent)
    if has_one_child_folder and count == 0 then
        return true
    elseif has_no_siblings then
        return true
    else
        return false
    end
end

The CI keeps failing with following error:

{
    "file:///home/runner/work/nvim-tree.lua/nvim-tree.lua/lua/nvim-tree/actions/tree/modifiers/expand-all.lua": [
        {
            "code": "redundant-parameter",
            "message": "This function expects a maximum of 2 argument(s) but instead it is receiving 3.",
            "range": {
                "end": {
                    "character": 43,
                    "line": 33
                },
                "start": {
                    "character": 37,
                    "line": 33
                }
            },
            "severity": 2,
            "source": "Lua Diagnostics."
        }
    ]
}

Any idea how to fix it?

ghostbuster91 · 2024-09-01T21:58:43Z

lua/nvim-tree/explorer/node.lua

@@ -36,7 +36,7 @@ end
 ---@param node Node
 ---@return boolean
 function M.has_one_child_folder(node)
-  return #node.nodes == 1 and node.nodes[1].nodes and vim.loop.fs_access(node.nodes[1].absolute_path, "R") or false
+  return node.nodes ~= nil and #node.nodes == 1 and node.nodes[1].nodes and vim.loop.fs_access(node.nodes[1].absolute_path, "R") or false


thought: guarding against calling this function with a file node

alex-courtis · 2024-09-02T02:04:38Z

This minimal setup worked nicely.

I've changed things a bit to use more api and less internals.

However, it's still using lib and explorer_node which are internals, subject to change at any time. They most definitely are changing during https://github.com/orgs/nvim-tree/projects/1

The user must be able to write their function via API only.

vim.g.loaded_netrw = 1
vim.g.loaded_netrwPlugin = 1

vim.cmd([[set runtimepath=$VIMRUNTIME]])
vim.cmd([[set packpath=/tmp/nvt-min/site]])
local package_root = "/tmp/nvt-min/site/pack"
local install_path = package_root .. "/packer/start/packer.nvim"
local function load_plugins()
  require("packer").startup({
    {
      "wbthomason/packer.nvim",
      "nvim-tree/nvim-tree.lua",
      "nvim-tree/nvim-web-devicons",
      -- ADD PLUGINS THAT ARE _NECESSARY_ FOR REPRODUCING THE ISSUE
    },
    config = {
      package_root = package_root,
      compile_path = install_path .. "/plugin/packer_compiled.lua",
      display = { non_interactive = true },
    },
  })
end
if vim.fn.isdirectory(install_path) == 0 then
  print("Installing nvim-tree and dependencies.")
  vim.fn.system({ "git", "clone", "--depth=1", "https://github.com/wbthomason/packer.nvim", install_path })
end
load_plugins()
require("packer").sync()
vim.cmd([[autocmd User PackerComplete ++once echo "Ready!" | lua setup()]])
vim.opt.termguicolors = true
vim.opt.cursorline = true

local api = require("nvim-tree.api")
local lib = require("nvim-tree.lib")
local explorer_node = require("nvim-tree.explorer.node")

local function expand_until_non_single(count, node, populate_node)
  populate_node(node)
  if node.nodes == nil or not node.parent.open then
    return false
  end
  local has_one_child_folder = explorer_node.has_one_child_folder(node)
  local has_no_siblings = explorer_node.has_one_child_folder(node.parent)
  if has_one_child_folder and count == 0 then
    return true
  elseif has_no_siblings then
    return true
  else
    return false
  end
end

local function expand_until()
  local node = api.tree.get_node_under_cursor()
  if node.open then
    lib.expand_or_collapse(node, nil)
  else
    if node.nodes then
      api.tree.expand_all(node, expand_until_non_single)
    else
      api.node.open.edit(node)
    end
  end
end

local function on_attach(bufnr)
  local function opts(desc)
    return { desc = "nvim-tree: " .. desc, buffer = bufnr, noremap = true, silent = true, nowait = true }
  end

  api.config.mappings.default_on_attach(bufnr)

  vim.keymap.set("n", "O", expand_until, opts("Expand until"))
end

_G.setup = function()
  require("nvim-tree").setup({
    on_attach = on_attach,
  })
end

alex-courtis · 2024-09-02T02:10:41Z

Suggestions:

lib.expand_or_collapse(node, nil)

Replace with node.open.edit after testing for children i.e. node.nodes

  local has_one_child_folder = explorer_node.has_one_child_folder(node)
  local has_no_siblings = explorer_node.has_one_child_folder(node.parent)

Write your own test in this case. It will be a lot simpler as you have tested most of the conditions already.

alex-courtis · 2024-09-02T02:14:51Z

Any idea how to fix it?

status is not a parameter for function Explorer:expand(node), looks like it's unused.

alex-courtis · 2024-09-02T02:15:58Z

That parameter is not used and thus can be removed.

alex-courtis

This is looking good, we'll get there:

simplify expand-all.lua preserving existing structure
remove unused variables etc. as per luals warnings
remove any usages of internals like lib
add options to api.tree.expand_all, documenting in help and api.lua
add an example to help, the absolute bare minimum, try to keep to 10 lines. If not, we can add to the wiki.

lua/nvim-tree/actions/tree/modifiers/expand-all.lua

alex-courtis · 2024-09-02T02:41:03Z

lua/nvim-tree/actions/tree/modifiers/expand-all.lua

          expansion_count = expansion_count + 1
-          expand(node)
+          populate_node(node)


I'm not quite following the logic here, but it looks like we're not actually making many changes e.g.

expand was renamed to populate_node

unnecessary should_expand boolean was extracted

get_last_group_node was moved out of expand

Please can you refactor this down to the minimum number of changes necessary. Integration with the existing structure will result in simpler code that is more easily reviewable and maintainable.

I appreciate that we've been through many revisions to get to this point, with complexity from earlier commits removed, however we need to revert some structural changes. Perhaps consider how you'd make these changes from scratch, knowing what you have learned over the course of this PR.

Please can you refactor this down to the minimum number of changes necessary. Integration with the existing structure will result in simpler code that is more easily reviewable and maintainable.

I totally agree with such approach. I think I was able to cut down amount of changes to the bare minimum.

There is one thing that I would like to discuss - changes in the recursor function. Before, when the logic was only to expand everything recursively, the condition was to check if node.open==true (putting expansion_count aside). In my case that is invalid as some nodes have been opened but they shouldn't be traversed further.

In both cases reusing should_expand function results in correct behavior. First of all, this doesn't have to be true in general. Maybe sometimes the logic for should_recurse will be different than the logic for should_expand.

Second. So, for each node we call should_expand twice, once to check if we should expand and second time to check if we should recurse. Between these calls, if should should_expand returns true, we call expand that changes the state of a given node. This means that users must not relay on node.open in their should_expand implementation as this would result in incorrect behavior.

I don't have enough experience in developing nvim plugins to judge if this is a problem, but wanted to bring this to your attention. Maybe this should at least be mentioned in the documentation?

In both cases reusing should_expand function results in correct behavior. First of all, this doesn't have to be true in general. Maybe sometimes the logic for should_recurse will be different than the logic for should_expand.

I like the consistency, it's more reason-able and will make maintenance easier.

This means that users must not relay on node.open in their should_expand implementation as this would result in incorrect behavior.

So long as we document that we are providing a consistent user experience.

Is node.nodes correct; can they test that?

ghostbuster91 · 2024-09-03T07:05:47Z

That parameter is not used and thus can be removed.

Right, I always forget that this is possible in dynamically-typed languages 👍

ghostbuster91 · 2024-09-03T09:48:39Z

Any idea how to fix it?

status is not a parameter for function Explorer:expand(node), looks like it's unused.

It must have changed once I rebased on latest master. Fixed 👍

ghostbuster91 · 2024-09-03T09:50:43Z

With all these changes my custom should expand function looks as follow:

local function has_one_child_folder(node)
    return #node.nodes == 1 and node.nodes[1].nodes and
        vim.loop.fs_access(node.nodes[1].absolute_path, "R") or false
end

local function expand_until_non_single(_, node, populate_node)
    populate_node(node)
    if node.nodes == nil or not node.parent.open then
        return false
    end
    return has_one_child_folder(node.parent)
end

Thank you for helping me getting this PR across the finish line. I truly appreciate your time and attitude 🙇‍♂️

alex-courtis · 2024-09-07T03:16:34Z

status is not a parameter for function Explorer:expand(node), looks like it's unused.

It must have changed once I rebased on latest master. Fixed 👍

Sorry about that; we are doing some significant refactoring of explorer/reload etc.

alex-courtis

This is looking incredible - the sample gives the user exactly what they need without overwhelming them.

I hope you don't mind: in the interest of speed I've pushed a couple of doc / example tweaks.

Address possibly unused param: feat: Allow to expand until certain condition is met #2790 (comment)
Issue: Sibling directories are not expanded:

mkdir -p foo1/bar/baz/foo/bar/baz
mkdir -p foo2/bar/baz/foo/bar/baz

E at root only expands one level deep:

Z at root expands foo1 correctly but does not expand foo1:

alex-courtis · 2024-09-07T04:29:20Z

doc/nvim-tree-lua.txt

+        end
+
+        -- on_attach
+        vim.keymap.set('n', 'Z', api.tree.expand_all(node, { expand_until = my_expand }))


I needed to wrap this in a function:

vim.keymap.set("n", "Z", function() api.tree.expand_all(nil, { expand_until = my_expand_until }) end, opts("My Expand All"))

or

local function my_expand_all() api.tree.expand_all(nil, { expand_until = my_expand_until }) end --- vim.keymap.set("n", "Z", my_expand_all, opts("My Expand All"))

We've hit two edges in our documentation here:

We don't specify how to inject a node when there are {opts} - this is the first one.

We don't give instructions on the need to use a function when parameters are needed.

That's OK - we can add those in a follow up PR to keep this one clear: #2899

Pushed minimal doc update.

alex-courtis · 2024-09-07T05:02:52Z

lua/nvim-tree/actions/tree/modifiers/expand-all.lua

@@ -47,13 +59,14 @@ local function gen_iterator()
    Iterator.builder(parent.nodes)
      :hidden()
      :applier(function(node)
-        if should_expand(expansion_count, node) then
+        if should_expand(expansion_count, node, populate_node) then


This appears unused. Can we remove it?

ghostbuster91 · 2024-10-09T09:43:56Z

@alex-courtis just fyi: I didn't forget about this. Just trying to finish my new apartment, so I can finally move in. I will try to finish this soon.

ghostbuster91 mentioned this pull request Jun 1, 2024

Allow nodes to expand until a certain condition is met. #2789

Open

ghostbuster91 added 2 commits September 1, 2024 16:19

Add new parameter to expand function

cd96cf9

feat: Add more controll to expand-all function

9f0a224

ghostbuster91 force-pushed the expand_until branch from 8cdcde4 to 9f0a224 Compare September 1, 2024 14:19

ghostbuster91 added 7 commits September 1, 2024 19:53

Working better

f691687

Remove print statements

7cf4cf8

Pass function to populate_node to userland

e8c4ce4

format code

4510332

Call populate_node in case it wasn't called by the user

a3f1588

Ignore unused variable

e0e12a4

Remove iterator from should_expand

96ffae1

ghostbuster91 commented Sep 1, 2024

View reviewed changes

alex-courtis requested changes Sep 2, 2024

View reviewed changes

ghostbuster91 added 5 commits September 3, 2024 09:07

Cleanup code from redundant stuff

7bda685

Add documentation

06aad16

Try to minimize the changes

57af3c4

Minimize amount of changes further

38d9e07

Remove redundant condition

b506e05

gvolpe added a commit to gvolpe/neovim-flake that referenced this pull request Sep 6, 2024

nvim-tree: add expand all mapping from nvim-tree/nvim-tree.lua#2790

1058973

alex-courtis added 2 commits September 7, 2024 15:01

luals types for expand_until, rename variable for consistency

b67209a

doc format and example clarification

3c093d4

alex-courtis requested changes Sep 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow to expand until certain condition is met #2790

feat: Allow to expand until certain condition is met #2790

ghostbuster91 commented Jun 1, 2024 •

edited

Loading

alex-courtis commented Jun 7, 2024 •

edited

Loading

alex-courtis commented Aug 25, 2024

ghostbuster91 commented Sep 1, 2024 •

edited

Loading

ghostbuster91 commented Sep 1, 2024

ghostbuster91 Sep 1, 2024

alex-courtis commented Sep 2, 2024 •

edited

Loading

alex-courtis commented Sep 2, 2024

alex-courtis commented Sep 2, 2024

alex-courtis commented Sep 2, 2024

alex-courtis left a comment •

edited

Loading

alex-courtis Sep 2, 2024

ghostbuster91 Sep 3, 2024 •

edited

Loading

alex-courtis Sep 7, 2024

ghostbuster91 commented Sep 3, 2024

ghostbuster91 commented Sep 3, 2024

ghostbuster91 commented Sep 3, 2024

alex-courtis commented Sep 7, 2024

alex-courtis left a comment •

edited

Loading

alex-courtis Sep 7, 2024 •

edited

Loading

alex-courtis Sep 7, 2024

ghostbuster91 commented Oct 9, 2024

feat: Allow to expand until certain condition is met #2790

Are you sure you want to change the base?

feat: Allow to expand until certain condition is met #2790

Conversation

ghostbuster91 commented Jun 1, 2024 • edited Loading

alex-courtis commented Jun 7, 2024 • edited Loading

alex-courtis commented Aug 25, 2024

ghostbuster91 commented Sep 1, 2024 • edited Loading

ghostbuster91 commented Sep 1, 2024

ghostbuster91 Sep 1, 2024

Choose a reason for hiding this comment

alex-courtis commented Sep 2, 2024 • edited Loading

alex-courtis commented Sep 2, 2024

alex-courtis commented Sep 2, 2024

alex-courtis commented Sep 2, 2024

alex-courtis left a comment • edited Loading

Choose a reason for hiding this comment

alex-courtis Sep 2, 2024

Choose a reason for hiding this comment

ghostbuster91 Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

alex-courtis Sep 7, 2024

Choose a reason for hiding this comment

ghostbuster91 commented Sep 3, 2024

ghostbuster91 commented Sep 3, 2024

ghostbuster91 commented Sep 3, 2024

alex-courtis commented Sep 7, 2024

alex-courtis left a comment • edited Loading

Choose a reason for hiding this comment

alex-courtis Sep 7, 2024 • edited Loading

Choose a reason for hiding this comment

alex-courtis Sep 7, 2024

Choose a reason for hiding this comment

ghostbuster91 commented Oct 9, 2024

ghostbuster91 commented Jun 1, 2024 •

edited

Loading

alex-courtis commented Jun 7, 2024 •

edited

Loading

ghostbuster91 commented Sep 1, 2024 •

edited

Loading

alex-courtis commented Sep 2, 2024 •

edited

Loading

alex-courtis left a comment •

edited

Loading

ghostbuster91 Sep 3, 2024 •

edited

Loading

alex-courtis left a comment •

edited

Loading

alex-courtis Sep 7, 2024 •

edited

Loading