feat(#2994): add visual selection operations#3268
feat(#2994): add visual selection operations#3268v3ceban wants to merge 1 commit intonvim-tree:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds first-class visual selection support to nvim-tree, allowing users to use V + motion to select a range of nodes and operate on them with a single keypress. The implementation includes support for toggle bookmark, copy, cut, delete, and trash operations on visually selected nodes.
Changes:
- Added visual mode keybindings (
m,c,x,d,D) for operating on visual selections - Implemented
Explorer:get_nodes_in_range()to retrieve all nodes within a visual selection - Created bulk operation functions
bulk_delete_nodes()andbulk_trash_nodes()with single confirmation prompts - Added descendant filtering to prevent errors when both a directory and its children are selected
- Updated help system to display visual mode keymaps with
[v]prefix
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lua/nvim-tree/marks/init.lua | Adds filter_descendant_nodes, bulk_delete_nodes, and bulk_trash_nodes functions for visual selection operations |
| lua/nvim-tree/explorer/init.lua | Implements get_nodes_in_range method to retrieve nodes within a line range |
| lua/nvim-tree/api/impl/post.lua | Adds wrap_visual_range and wrap_visual_bulk wrapper functions and wires up visual API endpoints |
| lua/nvim-tree/keymap.lua | Defines default visual mode keymaps (x mode) for visual operations |
| lua/nvim-tree/help.lua | Prefixes visual mode keymaps with [v] in help window |
| lua/nvim-tree/_meta/api/marks.lua | Adds type annotation for toggle_visual function |
| lua/nvim-tree/_meta/api/fs.lua | Adds type annotations for visual, cut_visual, remove_visual, and trash_visual functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I see many Copilot comments on this PR. Was AI used to generate this code? |
Yeah, it's just the review thingy. Didn't even know it'd work in someone else's repo.
A bit. Initially, to find the right API functions in the nvim-tree codebase for the bindings that I left in #2994, and then for the automated review, obviously. |
|
You can fix help generation by adding other mode handling to the script: diff --git a/scripts/help-defaults.sh b/scripts/help-defaults.sh
index 11ffefe..609072f 100755
--- a/scripts/help-defaults.sh
+++ b/scripts/help-defaults.sh
@@ -45,7 +45,7 @@ sed -i -e "/${begin}/,/${end}/{ /${begin}/{p; r /tmp/ON_ATTACH_DEFAULT.lua
# help human
echo > /tmp/ON_ATTACH_DEFAULT.help
-sed -E "s/^ *vim.keymap.set\(\"n\", \"(.*)\",.*api(.*),.*opts\(\"(.*)\".*$/'\`\1\`' '\3' '|nvim_tree.api\2()|'/g
+sed -E "s/^ *vim.keymap.set\(\".\", \"(.*)\",.*api(.*),.*opts\(\"(.*)\".*$/'\`\1\`' '\3' '|nvim_tree.api\2()|'/g
" /tmp/ON_ATTACH_DEFAULT.lua | while read -r line
do
eval "printf '%-17.17s %-26.26s %s\n' ${line}" >> /tmp/ON_ATTACH_DEFAULT.helpYou can then commit the updated help as per https://github.com/nvim-tree/nvim-tree.lua/blob/master/CONTRIBUTING.md#help-documentation |
Please don't use automated reviews in the future. I'll get to a review this week. |
|
This such a great feat, I was already looking forward to start working on this, I was just tired of jump from tree to oil and so on for some simple tasks. |
|
I haven't had a chance to review the code, however cursory initial testing looks good. This is fantastic as it opens the door for future visual (and other) mode operations. Looking at the new API like Rather than adding new API, we could make the existing API context dependent: when |
|
Disregard the CI that has not run; new test matricies have not yet filtered through. |
Thanks! And no worries - I’ve been quite busy myself - might not have a chance to jump on this again till tomorrow
Yeah, this sounds good to me. No reason to duplicate the apis if existing ones are ok to extend. I was a bit cautious to not break any direct api usage accidentally with the new mode, but the risk is probably low here. What should we do with mapping and help menu though? I assume having a separate entry for visual mode won’t make sense then. I can extend the description of existing maps to mention visual mode operations. Or, perhaps, we could even skip mentioning it in the description at all? At least to me this felt like a “natural” thing to try when I first attempted to delete multiple files. Mentioning it for every supported mapping may just add redundancy. sry for typos - sent from my phone |
Your caution is most gratefully appreciated. For the mapping itself, we can set multiple modes (table) e.g. vim.keymap.set({"n","v"}, "c", api.fs.copy.node, opts("Copy"))
We could be lazy with the help screen and just add a mode column in the middle, something like
That should be OK. vim help already has We can add a catch all note to |
Ok, this sounds good to me! I’ll get to it whenever I have a chance (most likely in the next ~24 hours) |
|
Cheers. No hurry at all. |
v3ceban
force-pushed
the
master
branch
7 times, most recently
from
698610b to
ba869c9
Compare
|
@alex-courtis Thanks for the patience with this! It's ready for review. |
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
This is looking good functionally:
- keymappings work nicely
- new visual operations work well
- help and doc are very clear, as are comments on the interfaces
However, the code requires significant rework. There have been far too many changes, refactors that are not necessary and the bulk FS operations are not in the correct class.
This change should be a single, atomic commit that is easy to read and identify the changes.
This change should do exactly one thing: add the 4 visual operations. Refactoring or reworking of unrelated code is encouraged, however not when it is unrelated to or not necessary for a change. Raise a separate, follow up PR.
Please:
- revert changes to
Marks. I don't think any changes are required: https://github.com/nvim-tree/nvim-tree.lua/pull/3268/changes#r2839038833 - move all the new visual api.fs operations to
actions.fsfiles
v3ceban
force-pushed
the
master
branch
4 times, most recently
from
0ff2441 to
cae607a
Compare
v3ceban
force-pushed
the
master
branch
13 times, most recently
from
4d5ce59 to
85d55bb
Compare
I wasn't able to replicate the exact issue you described, but it led me to discover a related problem I hadn't tested — pasting files that already exist at the destination triggered a separate prompt for each file, which is the same kind of prompt spam we wanted to avoid. To fix this, I updated the |
No worries, thank you for taking the time to review this in the first place. Appreciate all the feedback. I believe I did address your comments - let me know if anything is still missing, incorrect, or requires a refactor. |
|
Thanks for the updates, I'll get to a review shortly. |
That's a good call. Users will be doing more bulk pastes now that visual mode is available and we should harden it, with the skip option quite likely to be used. The rename suffix is a better option than rename. |
There was a problem hiding this comment.
Looking great, only a few small changes required.
Please
Preparing for release:
- You'll be "on call" for this change and will be mentioned in an issue/discussion
- If there is a breaking issue, we prefer to push a fix fast
- If that's not safe or achievable, we will have to roll back and redo the change with the fix
| ---Wrap a function to be mode-dependent: in visual mode, pass all nodes in the | ||
| ---visual range; in normal mode, pass a single node. The implementation decides | ||
| ---how to handle each case. |
| ---@return string prompt_input | ||
| ---@return string[] items_short | ||
| ---@return string[] items_long | ||
| function M.confirm_prompt(prompt_select, default_yes) |
There was a problem hiding this comment.
Very nice, this can be used for future functionality.
|
|
||
| ---@param node_or_nodes Node|Node[] | ||
| function M.fn(node_or_nodes) | ||
| if type(node_or_nodes) == "table" and node_or_nodes.is then |
There was a problem hiding this comment.
Good work checking for a table - my comment completely forgot to do that.
This tests for the presence of the is method: that node_or_nodes is a class. This is necessary as we may be passed a table that is not a class. We do still need to check that it is an instance of the node class - a third condition: node_or_nodes:is(Node) see :help nvim-tree-class
Same applies to trash, clipboard and marks.
Further api sanitization is well outside the scope of this issue and will be addressed by #2668
| end | ||
|
|
||
| ---@public | ||
| ---@private |
There was a problem hiding this comment.
Good work changing the visibility.
| end | ||
| end | ||
| return filtered | ||
| end |
There was a problem hiding this comment.
Using path mapping is working for most cases, however we need to account for the edge cases:
- windows: wsl and powershell with its variable silly C:/ etc.
- directory and file symlinks
Instead of matching the paths, we can directly query the node hierarchy. Something like this overly verbose sample:
for _, node in ipairs(nodes) do
log.line("dev", "nodes = %s", node.absolute_path)
end
---Filter function: is a node a descendent of nodes?
---@param node Node
---@return boolean true when node.parent is not present in nodes
local function is_descendent_of_nodes(node)
return not vim.tbl_contains(nodes, node.parent)
end
local filtered2 = vim.tbl_filter(is_descendent_of_nodes, nodes)
for _, node in ipairs(filtered2) do
log.line("dev", "filtered2 = %s", node.absolute_path)
endloops or filter functions or inline filters? There is no preference - it's a matter of personal taste, similar to java.
There was a problem hiding this comment.
Replaced path-string prefix matching with node.parent hierarchy traversal, as suggested. It still walks the full parent chain (not just one level) to correctly handle group_empty grouped nodes and collapsed directories where parents are not in the visual selection.
| end | ||
| end | ||
| return nodes | ||
| end |
There was a problem hiding this comment.
Tested OK for all 3 cases including {root_folder_label} false
| ' /tmp/ON_ATTACH_DEFAULT.lua | while read -r mode lhs apipath desc | ||
| do | ||
| eval "printf '%-17.17s %-26.26s %s\n' ${line}" >> /tmp/ON_ATTACH_DEFAULT.help | ||
| printf ' %-17.17s %-4.4s %-26.26s %s\n' "\`${lhs}\`" "${mode}" "${desc}" "|nvim_tree.api${apipath}()|" >> /tmp/ON_ATTACH_DEFAULT.help |
There was a problem hiding this comment.
That new loop is far safer / more readable than the eval.
| sed -E "s/^ *vim.keymap.set\(\"n\", \"(.*)\",.*api(.*),.*opts\(\"(.*)\".*$/'\`\1\`' '\3' '|nvim_tree.api\2()|'/g | ||
| " /tmp/ON_ATTACH_DEFAULT.lua | while read -r line | ||
| sed -E ' | ||
| s/^ *vim\.keymap\.set\(\{([^}]+)\}, *"([^"]+)",.*api(.*),.*opts\("([^"]*)".*/\1 \2 \3 \4/ |
There was a problem hiding this comment.
Works for edge cases and variations e.g.
vim.keymap.set({ "n", "x", "i", }, "d", api.fs.remove, opts("Delete"))
vim.keymap.set({ "n", "x", }, "d", api.fs.remove, opts("Delete"))
There was a problem hiding this comment.
This is working well, however there is a nondeterministic ordering of the mode, which looks a bit odd as the ns are not lined up.
┌─────────────────────────────────────────────┐
│nvim-tree mappings exit: q│
│ sort by description: s│
│ <2-LeftMouse> n Open │
│ <2-RightMouse> n CD │
│ <C-]> n CD │
│ <C-E> n Open: In Place │
│ <C-K> n Info │
│ <C-R> n Rename: Omit Filename │
│ <C-T> n Open: New Tab │
│ <C-V> n Open: Vertical Split │
│ <C-X> n Open: Horizontal Split │
│ <BS> n Close Directory │
│ <CR> n Open │
│ <Del> xn Delete │
│ <Tab> n Open Preview │
│ . n Run Command │
│ - n Up │
│ > n Next Sibling │
│ < n Previous Sibling │
│ ? n Help │
│ B n Toggle Filter: No Buffer │
│ C n Toggle Filter: Git Clean │
│ D xn Trash │
│ E n Expand All │
│ F n Live Filter: Clear │
│ H n Toggle Filter: Dotfiles │
│ I n Toggle Filter: Git Ignored│
│ J n Last Sibling │
│ K n First Sibling │
│ L n Toggle Group Empty │
This is overly critical and a nitpik. It's completely up to you whether you'd like to address it. More for your general knowledge.
It looks like the modes returned from vim.api.nvim_buf_get_keymap might not be deterministically ordered, despite our passing an ordered list.
We can sort and merge a string list like that using table.sort and .concat, which might make the merging a bit simpler.
Demo, put this in a file e.g. /tmp/foo.lua and :so it:
local modes = {
"x",
"n",
}
print("unsorted table = " .. vim.inspect(modes))
local unsorted = table.concat(modes, "")
print("unsorted string = " .. unsorted)
table.sort(modes)
print("sorted table = " .. vim.inspect(modes))
local sorted = table.concat(modes, "")
print("sorted string = " .. sorted)|
|
||
| -- mappings, left padded 1 | ||
| local fmt = string.format(" %%-%ds %%-%ds", max_lhs, max_desc) | ||
| local fmt = string.format(" %%-%ds %%-%ds %%-%ds", max_lhs, max_mode, max_desc) |
There was a problem hiding this comment.
Max is working correctly when there are mappings like { "v", "o", "s", "n", }
|
@alex-courtis Thanks again for the review! I’ll take a more detailed look and work on addressing the issues and edge cases you raised shortly. I’m okay with being mentioned in the issues. Will do my best to respond and create PRs with fixes in a timely manner :) |
v3ceban
force-pushed
the
master
branch
5 times, most recently
from
58c7b51 to
583600a
Compare
|
@alex-courtis Ready for re-review (no rush, I know you're busy!) Changes since last review:
|
Add visual selection operations
Closes #2994
See also: #2993
Summary
Adds visual selection support to nvim-tree. Users can visually select a range of nodes (charwise, linewise, or blockwise) and operate on them with a single keypress, matching standard Vim visual mode behavior.
Supported operations:
m)c)x)dandDel) -- single confirmation prompt for the entire selectionD) -- single confirmation prompt for the entire selectionDesign
Existing API functions (
api.fs.copy.node,api.fs.cut,api.fs.remove,api.fs.trash,api.marks.toggle) are now mode-dependent -- no new API functions are added. When called in visual mode they operate on the selected range; in normal mode they behave exactly as before.Default keymaps use
{ "n", "x" }mode tables so the same key works in both modes. Theg?help window shows a mode column (e.g.n,nx) for each mapping, with mode characters sorted for deterministic display.vim.fn.line("v")/vim.fn.line(".")while still in visual mode, avoiding<Esc>+'</'>marks. All visual and select modes are supported.Explorer:get_nodes_in_range()reuses the existingget_nodes_by_line()infrastructure.type(node_or_nodes) == "table" and node_or_nodes.is and node_or_nodes:is(Node)ensures correct handling when passed a table that is not a class instance.utils.filter_descendant_nodes): when a directory and its children are both selected, only the directory is operated on. Walks the full.parentchain to handlegroup_emptygrouped nodes and collapsed directories.ui.confirm.default_yesconfig; prompt construction extracted toutils.confirm_prompt().Clipboard:finish_paste()callsreload_explorer()to refresh the tree after paste. This ensures the tree reflects filesystem changes regardless of watcher state, and clears clipboard highlights even when all conflicts are skipped.nvim_feedkeyswith"nx"flags) before any operation executes.Files Changed
utils.lua--filter_descendant_nodes(),confirm_prompt(),is_visual_mode(),exit_visual_mode(),get_visual_nodes()api/impl/post.lua--wrap_node_or_visual()dispatcher for mode-dependent API functionsexplorer/init.lua--Explorer:get_nodes_in_range(start_line, end_line)actions/fs/remove-file.lua--remove_one()/remove_many()split,Nodeclass check dispatchactions/fs/trash.lua--trash_one()/trash_many()split,Nodeclass check dispatchactions/fs/clipboard.lua--bulk_clipboard(),resolve_conflicts(),finish_paste(),Nodeclass check dispatch incopy()/cut()marks/init.lua--toggle_one()extracted,toggle()handles single node or array withNodeclass checkkeymap.lua--{ "n", "x" }mode tables for visual-capable keymapshelp.lua-- mode column display, mode merging and sorting for duplicate lhs+desc entriesscripts/help-defaults.sh-- parse both"n"and{ "n", "x" }mode forms for help doc generation_meta/api/fs.lua,_meta/api/marks.lua-- updated docs with "In visual mode..." notesapi.lua-- mode-dependent behavior notedoc/nvim-tree-lua.txt-- regenerated help with mode column