-
-
Notifications
You must be signed in to change notification settings - Fork 609
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: add quit_on_open
and focus
opts to api.node.open.edit
#3054
base: master
Are you sure you want to change the base?
Conversation
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.
Works beautifully:
vim.keymap.set("n", "<S-CR>", function()
api.node.open.edit(nil, { quit_on_open = true })
end, opts("Open Quit"))
Handles directories, root etc. as expected.
For now I've added a draft commit that supports the first option and then just checks it and closes the tree view in the edit API method.
Not sure if that's the best option, but at first I decided to not forward it to the action level to not mix it with the quit_on_open global option that can be passed in to the action, and also to keep the changes restricted to the API code only.
I like that approach, it's really clear what's going on. The open_file
is far too complex and needs to be split up into file opening and post-actions; this is the first step.
end | ||
|
||
---@param mode string | ||
---@param toggle_group boolean? | ||
---@return fun(node: Node) | ||
---@return fun(node: Node, edit_opts: NodeEditOpts?) | ||
local function open_or_expand_or_dir_up(mode, toggle_group) | ||
---@param node Node |
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.
Missing param, CI got upset: https://github.com/nvim-tree/nvim-tree.lua/actions/runs/12975816599/job/36187330634?pr=3054
Needs
---@param edit_opts NodeEditOpts?
I recommend setting up wiki: Lua Language Server, which will show a warning about these sorts of things.
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.
Ok will do, thanks :)
If it helps, you're a member of the project and can now push branches directly, rather than your fork. I've added "fixes #1984" to the description: that issue will be automatically closed once this is merged. |
@GCrispino Hi! 👋 quick bit: instead of adding WIP to subject you can open draft PR which is a way clearer way to communicate. Congrats on your first PR here. :) |
Great! Will continue following this then 👍🏼
Oh yeah, totally forgot about this 😅 . Definitely easier to just push a new branch into the project, will do this on the next one, thanks for the reminder 👍🏼
Oh yeah sorry wasn't aware GitHub had draft PRs - set this one to be draft for now and removed the "WIP" prefix, thanks for the recommendation 👍🏼
Thank you! |
I believe this is now done, so removing the draft status. Something I'd like to note is that, since other methods on api.node (such as Would this be a problem, or if this isn't, would we need to add documentation anywhere for this? |
Wow... they do indeed. We'll need to test them all...
Proposal: add
|
Testing OK, one failure with I didn't have the energy to test all open variants... Can I please leave the function M.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", "?", api.tree.toggle_help, opts("Help"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.edit(nil, {})
end, opts("Edit"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.edit(nil, { quit_on_open = true, focus = false, })
end, opts("Edit Quit No Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.edit(nil, { quit_on_open = true, focus = true, })
end, opts("Edit Quit Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.edit(nil, { quit_on_open = false, focus = false, })
end, opts("Edit No Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.vertical(nil, {})
end, opts("Vert"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.vertical(nil, { quit_on_open = true, focus = false, })
end, opts("Vert Quit No Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.vertical(nil, { quit_on_open = true, focus = true, })
end, opts("Vert Quit Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.vertical(nil, { quit_on_open = false, focus = false, })
end, opts("Vert No Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.horizontal_no_picker(nil, {})
end, opts("Horiz No Picker"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.horizontal_no_picker(nil, { quit_on_open = true, focus = false, })
end, opts("Horiz No Picker Quit No Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.horizontal_no_picker(nil, { quit_on_open = true, focus = true, })
end, opts("Horiz No Picker Quit Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.horizontal_no_picker(nil, { quit_on_open = false, focus = false, })
end, opts("Horiz No Picker No Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.replace_tree_buffer(nil, {})
end, opts("Replace Tree Buffer"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.replace_tree_buffer(nil, { quit_on_open = true, focus = false, })
end, opts("Replace Tree Buffer Quit No Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.replace_tree_buffer(nil, { quit_on_open = true, focus = true, })
end, opts("Replace Tree Buffer Quit Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.replace_tree_buffer(nil, { quit_on_open = false, focus = false, })
end, opts("Replace Tree Buffer No Focus"))
-- these do work, even thought they do not make much sense
vim.keymap.set("n", "<S-CR>", function()
api.node.open.preview(nil, {})
end, opts("Preview"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.preview(nil, { quit_on_open = true, focus = false, })
end, opts("Preview Quit No Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.preview(nil, { quit_on_open = true, focus = true, })
end, opts("Preview Quit Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.preview(nil, { quit_on_open = false, focus = false, })
end, opts("Preview No Focus"))
-- FAIL
-- tree is not closed, but focus is correct
-- skip options for this one?
vim.keymap.set("n", "<S-CR>", function()
api.node.open.tab(nil, {})
end, opts("Tab"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.tab(nil, { quit_on_open = true, focus = false, })
end, opts("Tab Quit No Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.tab(nil, { quit_on_open = true, focus = true, })
end, opts("Tab Quit Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.tab(nil, { quit_on_open = false, focus = false, })
end, opts("Tab No Focus"))
-- these do work, even thought they do not make much sense
-- this function really should not be inside open
vim.keymap.set("n", "<S-CR>", function()
api.node.open.toggle_group_empty(nil, {})
end, opts("Toggle Group Empty"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.toggle_group_empty(nil, { quit_on_open = true, focus = false, })
end, opts("Toggle Group Empty Quit No Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.toggle_group_empty(nil, { quit_on_open = true, focus = true, })
end, opts("Toggle Group Empty Quit Focus"))
vim.keymap.set("n", "<S-CR>", function()
api.node.open.toggle_group_empty(nil, { quit_on_open = false, focus = false, })
end, opts("Toggle Group Empty No Focus"))
end |
quit_on_open
and focus
opts to api.node.open.edit
I'm fine with this proposal 👍🏼
Yeah sure, I just didn't have the time to give this another look.
|
if tabpage is not nil, then the function closes the tabpage in this specific tabpage
I just pushed some commits with the fix for the I still need to test the |
Sorry, yes, I should have mentioned. Really hacky.
Yes. It is very much needed, we just lack the time/people to select a framework and start building tests. There's a refactoring task for
|
-- new file is opened, however the previous one remains in the window
vim.keymap.set("n", "<S-CR>", function()
api.node.open.drop(nil, { quit_on_open = true, focus = false, })
end, opts("Drop No Focus"))
-- new file is opened, however the previous one remains in the window
vim.keymap.set("n", "<S-CR>", function()
api.node.open.drop(nil, { quit_on_open = true, focus = true, })
end, opts("Drop Quit Focus")) echo foo > foo
echo bar > bar
Repeat for |
CI is currently failing, see #3060 (comment) |
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.
Looking good!
You accidentally picked an issue that hits one of the most annoying, fragile parts of this codebase node.open_file.fn
:)
if not focus then | ||
-- if mode == "tabnew" a new tab will be opened and we need to focus back to the previous tab | ||
if mode == "tabnew" then | ||
vim.cmd(":tabprev") |
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.
vim.cmd(":tabprev") | |
vim.cmd.tabprev() |
When neovim gets around to exposing these as proper API we will be ready.
Hey @alex-courtis , apologies for the delay here, but I have been busy with other things in the past few weeks and haven't had the opportunity to test this further until today. So I did a few tests and couldn't come up with a fix for these scenarios with the About the It seems like when dropping the new nvim-tree window changes or something like that, and a different window is focused instead. For example, when following the steps below with the
Even when testing with nvim-tree without my modified code, I could reproduce this. This might be part of my lack of knowledge of vim's inner APIs, but I would expect that the second call to About the The nvim-tree window is closed indeed, but for some reason it doesn't open the new file (in your example, I don't know why that's the case, but I could accidentally find out that when putting an Do you have any idea of why this happened? 😅 |
There is never any pressure or rush; this is a community project with members all having limited availability. My window is also limited - usually Sun/Mon.
I like that trick... I'll definitely be using it in the future.
We're in difficult territory here; it's very hard to predict or react to window focus changes that nvim itself is making. Honestly, any workarounds usually end up being buggy and thus we avoid them. Events are sometimes not even sent, when debugging them. Proposal: limit the scope.We know which actions are safe and work, and some of the actions like preview do not make any sense. Limit the scope of this option to known good and desirable cases only. Proposed actions to remove:
Tab is the only special case, but it's rock solid. We'd need to document the new option for each function in Don't worry about |
That was mostly myself trying to compensate the lack of a proper debugging setup... do you have one for developing nvim plugins? I would love to have that set up on my machine.
got it 🙏
Ok that makes sense - most of the actions would probably not be used in this context anyway. But something wasn't clear to me: are you proposing to completely removing these actions? |
Don't try. We're adding unnecessary code paths that may indeed break with window focus etc. The "good" ones are fine - we know there are no unexpected focus changes and we are in complete control of focus.
I just add When I'm desparate I'll debug nvim and tree events, something like vim.api.nvim_create_autocmd("DirChanged", {
callback = function(data)
print(vim.inspect(data))
end,
})
-- tree
api.events.subscribe(api.events.Event.NodeRenamed, function(data)
log.line("dev", "Event.NodeRenamed %s %s", data.old_name, data.new_name)
end) |
Thanks for the tips on debugging :)
Ok that makes sense, was just trying to make sure I got it :) Should we then throw an error/warning or something to not let the user use these new options for the problematic actions? If you agree, I drafted that up in a separate branch. Can apply the commit in this one if you think it makes sense. GCrispino@1b51026#diff-69640a8794ab2834e7b05cd80b8c0516749f6e893813a81b4e71117f346e809fR276-R310 |
Sorry... I was vague... We should just ignore the new options available for the bad actions.
It looks like we can test |
As these options only apply to specific actions, we'll need to explicitly define the option for each e.g. node.open.vertical({node}, {opts}) *nvim-tree-api.node.open.vertical()*
|nvim-tree-api.node.edit()|, file will be opened in a new vertical split.
Parameters: ~
• {node} (Node|nil) file or folder
• {opts} (table) optional parameters
Options: ~
• {force} (boolean) delete even if buffer is modified, default false
• {quit_on_open} (boolean) TODO |
Ok that makes sense to me. I just have one question about the api methods and their corresponding modes - these methods usually have modes corresponding to their names (e.g. If we can, I believe I can implement this check just as you mentioned, otherwise I might have to do it outside of the scope of the |
Honestly, whatever is simplest and easiest for you to implement. New
wrapper, existing wrapper, new function etc.
This API file will be overhauled and all those common functions unrolled or
combined with the edit action.
We've accepted that API.lua will get worse before it gets better :)
…On Tue, 18 Feb 2025, 11:45 Gabriel Crispino, ***@***.***> wrote:
Should we then throw an error/warning or something to not let the user use
these new options for the problematic actions?
Sorry... I was vague...
We should just ignore the new options available for the bad actions.
edit( should only execute the if not focus then block when it's a known
good action e.g. edit, vertical etc.
It looks like we can test mode
Ok that makes sense to me.
I can do as you said and test if the value of mode corresponds to one of
the api methods you listed in this comment
<#3054 (comment)>
.
I just have one question about the api methods and their corresponding
modes - these methods usually have modes corresponding to their names (e.g.
Api.node.open.edit uses edit mode and Api.node.open.drop uses drop),
however Api.node.open.replace_tree_buffer uses edit_in_place.
My question then is that if we can safely check on the edit function for
mode edit_in_place for identifying if the current method being called is
Api.node.open.replace_tree_buffer.
If we can, I believe I can implement this check just as you mentioned,
otherwise I might have to do it outside of the scope of the edit function
(maybe a little bit like I did in the draft code available in the branch I
linked in my last comment). Does this make sense to you?
—
Reply to this email directly, view it on GitHub
<#3054 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALPQYXO7VKEEXQ4VQTSS4D2QJ7CXAVCNFSM6AAAAABV4Q3TR2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRUGI4TMNBVGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
[image: GCrispino]*GCrispino* left a comment
(nvim-tree/nvim-tree.lua#3054)
<#3054 (comment)>
Should we then throw an error/warning or something to not let the user use
these new options for the problematic actions?
Sorry... I was vague...
We should just ignore the new options available for the bad actions.
edit( should only execute the if not focus then block when it's a known
good action e.g. edit, vertical etc.
It looks like we can test mode
Ok that makes sense to me.
I can do as you said and test if the value of mode corresponds to one of
the api methods you listed in this comment
<#3054 (comment)>
.
I just have one question about the api methods and their corresponding
modes - these methods usually have modes corresponding to their names (e.g.
Api.node.open.edit uses edit mode and Api.node.open.drop uses drop),
however Api.node.open.replace_tree_buffer uses edit_in_place.
My question then is that if we can safely check on the edit function for
mode edit_in_place for identifying if the current method being called is
Api.node.open.replace_tree_buffer.
If we can, I believe I can implement this check just as you mentioned,
otherwise I might have to do it outside of the scope of the edit function
(maybe a little bit like I did in the draft code available in the branch I
linked in my last comment). Does this make sense to you?
—
Reply to this email directly, view it on GitHub
<#3054 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALPQYXO7VKEEXQ4VQTSS4D2QJ7CXAVCNFSM6AAAAABV4Q3TR2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRUGI4TMNBVGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
fixes #1984
As discussed on #1984 , this should implement the ability of specifying the following two options in the
api.node.open.edit
method:quit_on_open
: if set to true, it closes the tree when file related to the node is opened;focus
: if set totruefalse, opens the file but keep the focus on the tree. Similar to what's done in the preview feature, but actually creating a new buffer for each file.For now I've added a draft commit that supports the first option and then just checks it and closes the tree view in the edit API method.
Not sure if that's the best option, but at first I decided to not forward it to the action level to not mix it with the
quit_on_open
global option that can be passed in to the action, and also to keep the changes restricted to the API code only.Thoughts?