feat: delete entry from the picker without closing telescope

[dhruvmanila]

This will allow users to delete entries from the picker without closing telescope.

Discussion ref: #803 (review)
fixes: #689
fixes: #621
fixes: #672

This function in and of itself will just remove the entry, what I'm proposing next is:
Idea: Should we provide a callback parameter which will be called with the removed entry, so that the user or the specific picker can do exactly what needs to be done outside of telescope? Eg., delete the buffer, delete the file, etc.

The selection strategy is temporarily set to "row" as that will help keep the selection in the same position instead of resetting to the top. If there is any other way to achieve this, do suggest :)

Thanks @Conni2461 for helping!

delete_selection.mov

[dhruvmanila]

There is this problem:

• It seems to be breaking sometimes when deleting the entries in the middle of a search. I will take a look at it soon

[dhruvmanila]

Hi, @Conni2461 if you don't mind, can you help me out here, thanks if you do :)

There is one problem here: We cannot reliably find out the index in the results table for the current selection. I have tried the following methods:

• get_row() will return the current row of the selection which after getting filtered is not the same as that of results index.
• loop through the results and compare each entry with the selection table and get the index if it matches (this will only work if the reference is the same)
• loop through the results and compare the index field. It turns out only the async_static_finder creates the index field for each entry and not any of the other finders.

If there are any other ways to get the info, please do tell. Another option as done by async_static_finder would be to mark each result entry with an index field, thus getting the exact index of the selection in the results table.

[Conni2461]

I didn't know that index field was even a thing 🤣
I would just compare tbl refs. function EntryManager:find_entry(entry) does the same and that should work.

I have some other thoughts i need to evaluate and maybe test it but i am busy today. I try to find some time for it tomorrow :)

[dhruvmanila]

No problem, take your time and thanks for taking a look at it :)

I am using refs to find out the index of the selection in the results table and also provided an option delete_cb which will be called with the deleted selection. With this simple interface we can now define actions like:

actions.delete_buffer = function(prompt_bufnr)
  local current_picker = action_state.get_current_picker(prompt_bufnr)
  current_picker:delete_selection(function(selection)
    vim.api.nvim_buf_delete(selection.bufnr, { force = true })
  end)
end

-- could do the same for file_browser where we delete a file/directory

Also, we can think of doing multiple deletes using multi-select once this is well tested.

[tom-anders]

This looks really cool! I recently implemented something similar for my telescope-vim-bookmarks extension where I have an action that either deletes all currently multi-selected bookmarks or the bookmark under the cursor (if there's no multi selection). There I used the action.enhace{ post = ... } interface to just refresh the picker after deleting the bookmarks.

But this here seems much more convenient, looking forward to it being merged, especially with multi-selection support.

[tjdevries]

can we use self:refresh(...) inside of this call to re-use some of the code between these two items?

[dhruvmanila]

I believe it could be used but it closes the finder here: https://github.com/nvim-telescope/telescope.nvim/blob/master/lua/telescope/pickers.lua#L642

On that note, why are we closing the finder if the new finder is optional?

Edit: closing the finder only if the new finder is given will give us the ability to use the function self:refresh() instead of self:__on_lines(...)

[tjdevries]

Edit: closing the finder only if the new finder is given will give us the ability to use the function self:refresh() instead of self:__on_lines(...)

Yeah, we can probably make that change here if you want.

I'd prefer to re-use refresh (we can extend refresh in whatever ways we need to make that happen)

[dhruvmanila]

I am also not liking the way of finding the selection entry in the results, as mentioned here I tried a bunch of ways but comparing it directly in a for loop seems the only way to get the correct index. It's fine for single selection but in case of multi-selection, we would have to do compare each result entry with every selection from multi-select.

Is there any possibility where we add a special field for each entry in the result table identifying its position like result_idx or something like that? This would make it extremely easy to remove the entry.

Edit: async_static_finder already does it with index field.

[Conni2461]

I believe it could be used but it closes the finder here

Just bad programming by me 🤣

Sorry that i haven't looked at it earlier, it was a crazy week and then i wanted to look at it Saturday and the whole gitsigns.nvim + fzf-native ffi fiasco started. :)

Great work here. I just tested it. (Also i thought we could change the self.selection_strategy back with completion_callback, but i think do lines can run multiple times last time i check, so it might not work. Like being able to set a special completion_callback that gets poped after running it ones. Idk)

Multi selection seems like a good idea, (maybe as option that can be enabled/disabled or as section function). Maybe someone has a use case in the future where that person doesn't want it to be the whole multi selection.

[dhruvmanila]

I tested with completion_callback and it works although it gets added every time I call delete_selection and thus the callbacks keep increasing, currently there's no way to remove the called function.

We could reset the callback table, but then what about any existing implementation, extensions and user configs which are using this?

Something like this:

{ <function 1> }
{ <function 1>, <function 2> }
{ <function 1>, <function 2>, <function 3> }
{ <function 1>, <function 2>, <function 3>, <function 4> }
{ <function 1>, <function 2>, <function 3>, <function 4>, <function 5> }

Also, I am brainstorming a bit on how to do multi-selection and here's what I'm trying:

• Get the selections from self._multi:get(), if it is empty then make a table of the current entry
• Loop through the results and store the result index for the corresponding selection in a table
• Sort the result index table in reverse order and start removing them (reverse because removing an element shifts the other elements to close the hole)
• If selections were from multi select then reset self._multi it
• Refresh
• Reset the selection_strategy

I've been using this in the buffers picker to delete buffers right when I opened this PR

Screen.Recording.2021-05-17.at.22.48.19.mov

[Conni2461]

I tested with completion_callback and it works although it gets added every time I call delete_selection and thus the callbacks keep increasing, currently there's no way to remove the called function.

Yeah i meant adding something that would pop the function after executing but we can just keep vim.schedule for now.

Do you want to integrate both your prs together? or whats your plan?

[dhruvmanila]

Do you want to integrate both your prs together? or whats your plan?

Yeah, I will close my other PR and add the same here.

[Conni2461]

LGTM Thanks :)

But there are merge conflicts. Sorry :)

[Conni2461]

What about realizing #582 now? To make everything simpler for end users and we can be confident in not mapping things.

I just did a super simple prototype that works, missing theme and mappings. Can anyone think of a better interface than tamis proposal? (Also i am not sure about the - new custom picker thingy but should be doable as well)

[Conni2461]

Re: I think this would make the experience of setting a new action for a specific picker pretty good: #883

[dhruvmanila]

of course, the possible function to map (with an example mapping) should be documented

Yeah that sounds good. Should I add the documentation to the :help docs as I don't see any reference to actions in the README? Also, with the changes from #883, should we wait until that merges?

Re: I think this would make the experience of setting a new action for a specific picker pretty good: #883

This is really good work and would make configuring Telescope simple and intuitive!

[Conni2461]

I added documentation for #883. I would ask tj what he thinks and then we can merge yours either before mine or after. Doesn't really matter but this way we only really need to document the new way :)

[dhruvmanila]

Sounds good to me 👍

[Conni2461]

I'll test it with my configuration PR that i just merged and then i'll merge this and update the comment on how to set it up.

WORKS
Example config:

require("telescope").setup {
  pickers = {
    buffers = {
      show_all_buffers = true,
      sort_lastused = true,
      theme = "dropdown",
      previewer = false,
      mappings = {
        i = {
          ["<c-d>"] = "delete_buffer",
        }
      }
    }
  }
}

[Conni2461]

Thank you amazing feature :) and thanks everyone for being patient that long

[dhruvmanila]

Awesome, thanks everyone for their guidance and feedback! :)

Now, onto formatting the config in the new way! :D

[mroavi]

Hi @dhruvmanila. Thank you for this, it's awesome! I was really missing an easy way of deleting buffers.

There is only one minor thing that is a bit annoying: if you delete the last entry, then the selection moves back to the first entry, while it would be much better if it stayed in the last. Should I open an issue for this?

[fdschmidt93]

Not needed, you can do something like this (just tested it, works)

local actions = require "telescope.actions"
require("telescope").setup {
  pickers = {
    buffers = {
      mappings = {
        i = {
          ["<c-d>"] = actions.delete_buffer + actions.move_to_top,
        }
      }
    }
  }
}

[mroavi]

I think you misunderstood. The default behavior is great: when you delete an entry, the selection stays where it is. The problem is that this rule is broken whenever you delete the last entry in the menu. In this case, it actually jumps back to the first entry while (in my opinion) it should stay in the last.

[l-kershaw]

@mroavi If I understand what you want correctly, you can achieve this by setting your scroll_strategy to limit, although this then affects your ability to cycle from one end of the list to another.

[mroavi]

@l-kershaw you are completely right. Didn't know about the possibility to configure scrolling behavior. Please ignore my suggestion.

[l-kershaw]

No worries 🙂

[fdschmidt93]

Yes, on second thought what I suggested is pretty off 😆 I took last too literally, cool you've got it solved!

[ssh352]

I'll test it with my configuration PR that i just merged and then i'll merge this and update the comment on how to set it up.

WORKS Example config:

require("telescope").setup {
  pickers = {
    buffers = {
      show_all_buffers = true,
      sort_lastused = true,
      theme = "dropdown",
      previewer = false,
      mappings = {
        i = {
          ["<c-d>"] = "delete_buffer",
        }
      }
    }
  }
}

Thanks, this however doesn't delete the current buffer even if you select to delete it.

[Kadrian]

For info, I was happy to find that mapping this to normal mode + d works as well

mappings = {
  n = {
    ["d"] = "delete_buffer",
  }
}

[sleewoo]

this check added after feature landing seems breaking stuff, at least for find_files and live_grep

telescope.nvim/lua/telescope/actions/init.lua

Line 1103 in 2d92125

local force = vim.api.nvim_buf_get_option(selection.bufnr, "buftype") == "terminal"

copied the function to my local setup, commented callback content and works well

[jamestrew]

@sleewoo I think you're probably trying to delete an entry that doesn't exist as a buffer. The delete_buffer action isn't really designed to work with pickers like find_files and live_grep since unless you've opened nearly every file in the cwd, most of the entry result from those pickers won't exist as buffers.

This action is probably best used with something like the buffers picker which lists all open buffers.

Probably could do with better error handling but considering the purpose of this action and the way you're using it, I think this is low-priority.

Also, by commenting out the contents of the callback function, you've broken the intended use case of the action -- deleting an open buffer.

[sleewoo]

@jamestrew yes, reverted my dirty hack as realized i try to delete buffers that does not exists.
what i was trying to do is out of scope for telescope.
thanks.

[yuhsienchiang]

Hello @dhruvmanila,
I'm quite new to neovim plugin implementation and lua, and recently I am implementing my own telescope extension for toggleterm. I ran into an issue and couldn't solve it. Could you give my some guides on how to keep the telescope open when deleting the terminal buffer?

My extension is to list out all the terminal buffers created using toggleterm. By pressing [CR], the selected terminal buffer(s) will be opened or focused. This part works fine.

I also added a keymap [C-d] to remove(closed) the terminals. The terminals can be removed(closed) successfully, however, the telescope window we be closed every time I press [C-d] to perform the action. I would like the remove action performs just like the actions.delete_buffer, which will not close telescope.

The code below is my implementation. I wrote my code based on your implementation of actions.delete_buffer, but still didn't get the desired result. Did I miss anything or this is a limitation for handling terminal buffers?

Thank you so much!

local terms = require("toggleterm.terminal")
local pickers = require("telescope.pickers")
local finders = require("telescope.finders")
local actions = require("telescope.actions")
local action_state = require("telescope.actions.state")
local conf = require("telescope.config").values

local M = {}

M.term_select = function(opts)
	opts = opts or {}
	local terminals = terms.get_all(true)
	pickers.new(opts, {
		prompt_title = "Toggleterm",
		finder = finders.new_table({
		    results = terminals,
		    entry_maker = function(entry)
			return {
			    value = entry,
			    text = tostring(entry.bufnr),
			    display = tostring(entry.id .. ": " .. entry:_display_name()),
			    ordinal = tostring(entry.id),
			}
		    end,
		}),
		sorter = conf.generic_sorter(opts),
		attach_mappings = function(prompt_bufnr, map)
		    actions.select_default:replace(function()

                    -- close the prompt window
		    actions.close(prompt_bufnr)

		    -- get the selected terminal, which is the table returned by entry_maker
		    local selection = action_state.get_selected_entry()
		    if selection == nil then
			return
		    end

		    -- perform action
		    if selection.value:is_open() then
			selection.value:focus()
		    else
			selection.value:open()
		    end
		end)

		map({ "i", "n" }, "<C-d>", function()
		    local current_picker = action_state.get_current_picker(prompt_bufnr)
		    current_picker:delete_selection(function(selection)
			local ok = pcall(vim.api.nvim_buf_delete, selection.value.bufnr, { force = true })
			return ok
						
		    end)
		end)

		return true
	    end,
	}):find()
end

return M

[ryanmsnyder]

I've confirmed the issue of telescope closing after a terminal buffer is deleted (using delete_selection with a callback) is specific to terminal buffers.

I've eliminated some possibilities for this problem. The issue doesn't seem to be in the delete_selection method directly (i.e. it isn't related to telescope removing the entry from the finder). It definitely is related to the actual deletion of the buffer that's provided in the callback to delete_selection - it's a result of calling vim.api.nvim_buf_delete. However, I haven't been able to figure out why deleting a terminal buffer causes telescope to close.

I also tried to determine if this is related to toggleterm. My thinking was that maybe there was some post-processing/cleanup from toggleterm after a terminal buffer was deleted that was causing the focus to be removed from telescope and, therefore, closing telescope. I was hopeful when I found that toggleterm listens for the TermClose event, which is fired by neovim when a terminal buffer is deleted. However, this didn't appear to be the culprit either.

I then assumed it had to do with neovim's handling of the deletion of terminal buffers. To test that assumption, I opened a terminal buffer by running the :terminal command (bypassing toggleterm). After retrieving the terminal buffer's bufnr (:echo bufnr('')), I opened toggleterm and switched to normal mode. With the telescope window still open, I deleted the terminal buffer (:lua vim.api.nvim_buf_delete(term_bufnr, { force = true })). I was surprised that the telescope window remained open.

It must have to do with toggleterm but I haven't been able to figure out what could be causing it.

What makes it more difficult to diagnose is that telescope isn't always closed after deleting a terminal buffer that was created with toggleterm. There are some scenarios where it doesn't close it but I haven't been able to identify a useful pattern.