feat: allow passing node to collapse all

[itsramiel]

Summary

Support passing node to collapse_all to allow collapsing all dirs under a given node instead of always root node.

Addresses #3132 by providing an api

[itsramiel]

@alex-courtis Please review and lmk if you have any comments. Then I'll update the api docs

[alex-courtis]

I saw the issue before seeing this PR.

Please use an opts parameter as per draft #3132 (comment)

[alex-courtis]

Many thanks for the fast PR. Please:

• handle legacy api calls feat: allow passing node to collapse all #3133 (comment)
• use opts - my apologies, I advised you incorrectly Collapse directories under current highlighted directory #3132 (comment)
• Snake case please node_at_cursor

[alex-courtis]

As we are changing the signature of this API, we're going to need to retain backwards compatibility, in the case of the user calling it like collapse_all(true)

See

nvim-tree.lua/lua/nvim-tree/actions/tree/toggle.lua

Lines 7 to 29 in 1ae1c33

	---Toggle the tree.
	---@param opts ApiTreeToggleOpts|nil|boolean legacy -> opts.find_file
	---@param no_focus string|nil legacy -> opts.focus
	---@param cwd boolean|nil legacy -> opts.path
	---@param bang boolean|nil legacy -> opts.update_root
	function M.fn(opts, no_focus, cwd, bang)
	-- legacy arguments
	if type(opts) == "boolean" then
	opts = {
	find_file = opts,
	}
	if type(cwd) == "string" then
	opts.path = cwd
	end
	if type(no_focus) == "boolean" then
	opts.focus = not no_focus
	end
	if type(bang) == "boolean" then
	opts.update_root = bang
	end
	end
	opts = opts or {}

for a complex example.

[alex-courtis]

Test cases:

  vim.keymap.set("n", "nk", function()
    local node = api.tree.get_node_under_cursor()
    api.tree.collapse_all(node, true)
  end, opts("Collapse Node Keep"))

  vim.keymap.set("n", "nn", function()
    local node = api.tree.get_node_under_cursor()
    api.tree.collapse_all(node, false)
  end, opts("Collapse Node No Keep"))

  vim.keymap.set("n", "ak", function()
    api.tree.collapse_all(nil, true)
  end, opts("Collapse All Keep"))

  vim.keymap.set("n", "an", function()
    api.tree.collapse_all(nil, false)
  end, opts("Collapse All No Keep"))

  -- collapse-all.lua:43: attempt to index local 'node' (a boolean value)
  vim.keymap.set("n", "lk", function()
    api.tree.collapse_all(true)
  end, opts("Collapse All Legacy Keep"))

  -- collapse-all.lua:43: attempt to index local 'node' (a boolean value)
  vim.keymap.set("n", "ln", function()
    api.tree.collapse_all(true)
  end, opts("Collapse All Legacy No Keep"))

Following addition of opts they should look something like:

  vim.keymap.set("n", "nk", function()
    local node = api.tree.get_node_under_cursor()
    api.tree.collapse_all(node, { keep_buffers = true, })
  end, opts("Collapse Node Keep"))

[alex-courtis]

Apologies, my time is limited, I'll look at this next weekend.

[alex-courtis]

Test cases passing:

  vim.keymap.set("n", "nk", function()
    local node = api.tree.get_node_under_cursor()
    api.tree.collapse_all(node, { keep_buffers = true })
  end, opts("Collapse Node Keep"))

  vim.keymap.set("n", "nn", function()
    local node = api.tree.get_node_under_cursor()
    api.tree.collapse_all(node, { keep_buffers = false })
  end, opts("Collapse Node No Keep"))

  vim.keymap.set("n", "ak", function()
    api.tree.collapse_all(nil, { keep_buffers = true })
  end, opts("Collapse All Keep"))

  vim.keymap.set("n", "an", function()
    api.tree.collapse_all(nil, { keep_buffers = false })
  end, opts("Collapse All No Keep"))

  vim.keymap.set("n", "lk", function()
    api.tree.collapse_all(true)
  end, opts("Collapse All Legacy Keep"))

  vim.keymap.set("n", "ln", function()
    api.tree.collapse_all(true)
  end, opts("Collapse All Legacy No Keep"))

[alex-courtis]

I hope you don't mind, but I pushed :help for this method.

We've unfortunately introduced an inconsistency in the way we handle the node param:

nvim-tree.lua/doc/nvim-tree-lua.txt

Lines 1688 to 1691 in 1ae1c33

	Functions accepting {node} as their first argument will use the node under the
	cursor when that argument is not present or nil.

expand_all will select the node under the cursor when expanding, however collapse_all does not.
If we do the same for collapse_all it will break current behaviour: users expect collapse_all to act on the entire tree.

Apologies for the repeated requests for changes, however we need to spend extra time on new API, as we cannot change it once published.

It seems that collapse and expand specific to nodes should be present in api.node rather than api.tree, which is not node specific. They could also operate in a similar manner to create-file, operating on the parent directory when a file is focused.

I'll get back to you...

[itsramiel]

Hi @alex-courtis

I am fine with taking a step back and rethinking how to implement that. Take your time.

On a side note. I appreciate how much effort you are putting to not create breaking changes and maintain backwards compat, but doesnt it sometime make sense to take the hard decision and introduce a breaking version with a major version? I mean if expand_all assumes the current node is the node to expand, doesnt it make sense to do the same with collapse_all to maintain consistency?

[alex-courtis]

On a side note. I appreciate how much effort you are putting to not create breaking changes and maintain backwards compat, but doesnt it sometime make sense to take the hard decision and introduce a breaking version with a major version?

That time will inevitably come, however it would be best to wait until we have a planned large feature / fix that must break API.

I mean if expand_all assumes the current node is the node to expand, doesnt it make sense to do the same with collapse_all to maintain consistency?

Exactly! Consistency is the hard part, and users have come to expect it.

Proposal after sleeping in it

For nodes

api.node.collapse

• Move the new collapse functionality to a new API function api.node.collapse
• Revert existing api.tree.collapse_all
• When api.node.collapse is passed a nil node, operate on the node_at_cursor
  • if that's a directory, collapse it
  • if it's a file, collapse the parent directory, like api.fs.create

api.node.expand

This is outside the scope of this change, however bonus points if you feel like implementing this.

• Create new API function api.node.expand
• It has the same functionality as api.tree.expand_all except
  • When it's passed a nil node and a directory is not under the cursor it will expand the parent directory, as per api.node.collapse

For tree

api.tree.collapse_all

This is consistent with other functions and doesn't need a change.

api.tree.expand_all

Using node under cursor or root is inconsistent, however any change would break users. If they want more control they can use the new api.node.expand.

Default Mappings

• Change C description from Collapse to Collapse All
• Add A Collapse
• Add X Expand

[alex-courtis]

Apologies for messing you around on this one.

Please tell me what you think of the proposal.

I think the minimum we need to do here is move the changes to api.node.collapse. Anything else is nice to have.

[itsramiel]

Apologies for messing you around on this one.

All good. That's software 👍

Please tell me what you think of the proposal.

I think it makes sense to have apis to act on a node under the node module 👍

I think the minimum we need to do here is move the changes to api.node.collapse. Anything else is nice to have.
I'll try to get to it as soon as I can. Of course if anybody would like to help, it is appreciated 🙏