-
-
Notifications
You must be signed in to change notification settings - Fork 626
feat(#3132): add api.node.expand and api.node.collapse #3133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(#3132): add api.node.expand and api.node.collapse #3133
Conversation
@alex-courtis Please review and lmk if you have any comments. Then I'll update the api docs |
I saw the issue before seeing this PR. Please use an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for the fast PR. Please:
- handle legacy api calls feat(#3132): add api.node.expand and api.node.collapse #3133 (comment)
- use opts - my apologies, I advised you incorrectly Collapse directories under current highlighted directory #3132 (comment)
- Snake case please
node_at_cursor
---@param keep_buffers boolean | ||
function M.fn(keep_buffers) | ||
function M.fn(node, keep_buffers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 {} | |
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 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")) |
Apologies, my time is limited, I'll look at this next weekend. |
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")) |
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
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 I'll get back to you... |
This reverts commit d243da3.
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 |
That time will inevitably come, however it would be best to wait until we have a planned large feature / fix that must break API.
Exactly! Consistency is the hard part, and users have come to expect it. Proposal after sleeping in itFor nodes
|
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 |
All good. That's software 👍
I think it makes sense to have apis to act on a node under the node module 👍
|
Any progress on this one @itsramiel ? Would you like me to complete this and merge it? No pressure... |
@alex-courtis I am busy this period so I cant get to it. Feel free to take it if you have the time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added expand and collapse, tested OK:
vim.keymap.set("n", "nk", function()
local node = api.tree.get_node_under_cursor()
api.node.collapse(node, { keep_buffers = true })
end, opts("Collapse Node Keep"))
vim.keymap.set("n", "nn", function()
local node = api.tree.get_node_under_cursor()
api.node.collapse(node, { keep_buffers = false })
end, opts("Collapse Node No Keep"))
vim.keymap.set("n", "ak", function()
api.tree.collapse_all({ keep_buffers = true })
end, opts("Collapse All Keep"))
vim.keymap.set("n", "an", function()
api.tree.collapse_all({ 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(false)
end, opts("Collapse All Legacy No Keep"))
vim.keymap.set("n", "en", function()
local node = api.tree.get_node_under_cursor()
api.node.expand(node)
end, opts("Expand Node"))
vim.keymap.set("n", "ea", function()
local node = api.tree.get_node_under_cursor()
api.tree.expand_all(node)
end, opts("Expand All Node"))
vim.keymap.set("n", "et", function()
api.tree.expand_all(nil)
end, opts("Expand All Tree"))
Summary
Support passing
node
tocollapse_all
to allow collapsing all dirs under a given node instead of always root node.Addresses #3132 by providing an api