Skip to content
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(#3037): add node API methods for deleting/wiping buffers #3040

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

GCrispino
Copy link

@GCrispino GCrispino commented Dec 30, 2024

This PR adds two API methods for respectively deleting and wiping the buffer for the current file under the tree cursor:

  • Api.node.buffer.delete; and
  • Api.node.buffer.wipe,

as discussed in #3037.

They both use the same underlying function defined in the new file added at lua/actions/node/utils.lua.
This function checks if there's an opened and loaded buffer for the file under the tree cursor.
If there isn't, it notifies an error message.

Otherwise, it deletes/wipes the buffer if it's not modified (unless opts.force is true)

I'm setting this as WIP because I haven't run the make commands yet.
EDIT: done

@GCrispino GCrispino changed the title WIP: feat(#2826): add node API methods for deleting/wiping buffers WIP: feat(#3037): add node API methods for deleting/wiping buffers Dec 30, 2024
@GCrispino GCrispino changed the title WIP: feat(#3037): add node API methods for deleting/wiping buffers feat(#3037): add node API methods for deleting/wiping buffers Dec 31, 2024
Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works nicely, I look forward to using it.

  • Revert ApiTreeToggleOpts change
  • Change warning level of no-loaded-buffer
  • Remove unused options delete_buffer and wipe_buffer
  • Refactor action methods, message to follow
  • Add help at 6.3 API NODE nvim-tree-api.node

@@ -131,7 +132,7 @@ end
Api.tree.open = wrap(actions.tree.open.fn)
Api.tree.focus = Api.tree.open

---@class ApiTreeToggleOpts
---@class ApiTreeToggleOptsApiTreeTo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this change? lua/nvim-tree/actions/tree/toggle.lua is still using this class...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea 😅 , this was probably done accidentally. Just reverted it 👍🏼

@@ -286,6 +287,16 @@ Api.node.navigate.diagnostics.prev_recursive = wrap_node(actions.moves.item.fn({
Api.node.navigate.opened.next = wrap_node(actions.moves.item.fn({ where = "next", what = "opened" }))
Api.node.navigate.opened.prev = wrap_node(actions.moves.item.fn({ where = "prev", what = "opened" }))

---@class ApiNodeDeleteWipeBufferOpts
---@field force boolean|nil default false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

@@ -470,6 +470,8 @@ local DEFAULT_OPTS = { -- BEGIN_DEFAULT_OPTS
remove_file = {
close_window = true,
},
delete_buffer = {},
wipe_buffer = {},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these options used?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I was unsure if it was a convention to have these even if there are no underlying options, so I added these in even though they are empty.

I can remove them 👍🏼

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested permutations and combination with :ls! reporting as expected.

  vim.keymap.set("n", "BW", function()
    api.node.buffer.wipe(api.tree.get_node_under_cursor(), { force = false })
  end, opts("Wipe"))

  vim.keymap.set("n", "BD", function()
    api.node.buffer.wipe(api.tree.get_node_under_cursor(), { force = true })
  end, opts("Delete"))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thank you 👍🏼

-- check if buffer for file at cursor exists and if it is loaded
local bufnr_at_filename = vim.fn.bufnr(filename)
if bufnr_at_filename == -1 or vim.fn.getbufinfo(bufnr_at_filename)[1].loaded == 0 then
notify.error("No loaded buffer coincides with " .. notify_node)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please change this to INFO? Users may wish to ignore this message.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sure! Should I also change the error statement at line 37 (the one that says "Buffer for file " .. notify_node .. " is modified"), or just this one?

@alex-courtis
Copy link
Member

alex-courtis commented Jan 13, 2025

I like the modularity, it's really easy to read.

Having 3 new files is not necessary, delete and wipe can be handled in one place:
Create a single file buffer.lua: containing the utils function and fn( methods renamed to wipe and delete.
We don't really need the fn() pattern any more.

These actions do act on a node, and belong in actions/node. Rather than the absolute_path being passed in, the node itself should be passed in:

  1. Prevents error when the user passes a node without that field.
  2. Allows testing of the node's type as a FileNode. We don't want to try and delete directories or the tree node itself. That can silently exit.

@GCrispino
Copy link
Author

GCrispino commented Jan 15, 2025

I like the modularity, it's really easy to read.

Having 3 new files is not necessary, delete and wipe can be handled in one place: Create a single file buffer.lua: containing the utils function and fn( methods renamed to wipe and delete. We don't really need the fn() pattern any more.

These actions do act on a node, and belong in actions/node. Rather than the absolute_path being passed in, the node itself should be passed in:

  1. Prevents error when the user passes a node without that field.
  2. Allows testing of the node's type as a FileNode. We don't want to try and delete directories or the tree node itself. That can silently exit.

@alex-courtis

Ok, when I get some free time I'll work on these changes. Thanks for the review :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants