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: chain buitlin commands #1544

Closed
wants to merge 1 commit into from

Conversation

kylo252
Copy link
Contributor

@kylo252 kylo252 commented Dec 3, 2021

Description

Allows chaining/invoking builtin commands by extending action.run_builtin

Fixes: #564, #930

Partially fixes: #618, #620, #1357

Example

Other useful examples

  • fuzzy_filter_results: works by chaining grep_string and assuming you have fzf as a sorter
mNiJTfMyzE.mp4
  • dynamic_filetype: works by modifying the vimgrep_args passed to ripgrep using the filetype under the cursor to either include or exclude it
skipfile.mp4

@Conni2461
Copy link
Member

Isnt this similar to #1115 which was more what i had in mind.

@kylo252
Copy link
Contributor Author

kylo252 commented Dec 3, 2021

Isnt this similar to #1115 which was more what i had in mind.

I think that this is a simpler but more flexible implementation. Notice that I didn't include anything other than the modification for run_builtin, you can still decide on having the others added it later or not.

Also taken from my discussion on Matrix with @fdschmidt93

don't hold your breath on #1115. There are a couple of good reasons as to why I haven't really continued on this. telescope-fzf-native easily allows refining with spaces between tokens and https://github.com/nvim-telescope/telescope-rs.nvim will maybe eventually pave the way that the entry manager will be fast enough, such that you just get all the lines from rg and use telescope-fzf-native to filter that.

@Conni2461
Copy link
Member

Conni2461 commented Dec 3, 2021

Okay i checked we already talked about this: https://matrix.to/#/!cxlVvaAjYkBpQTKumW:gitter.im/$xKv2oWfroDCASzP35yPfHWpLk-sk2zefOPRW-j4aoi4?via=matrix.org&via=gitter.im

I made the proposal that tj and I came up and fd opened a PR a little bit later. I dont think what fd says here made that much sense. There are good reasons why you dont want to get all lines and filter with fzf. fzf-native and sorters match on the whole ordinal which might include the filename and lnum and column. Also the raw-live-grep team is currently working on allowing to pass in options. Also regex is a thing that live_grep can do and grep_string on all lines, etc...
So i want something like a pipe. Not just that you can run from one builtin (live_grep) get the current prompt and then run grep_string.

I know you said that ripgrep is deterministic but its also unnecessary to re do the processing, if we already have the results. We can just put them back in with a additional sorter (and opts).

Then it can be used for all kinds of different things, and even realize the ivy thing. @elianiva worked on something like this quite some time ago: #793 (comment)

And in the end we do something like this in live_grep attach_mappings:

attach_mappings = function(map) 
  map("i", "<c-f>", function(prompt_bufnr)
    local line = action_state.get_current_line()
    require("telescope.actions.generate").pipe(prompt_bufnr, {
      prompt_title = "Find Word (" .. line .. ")" 
      sorter = conf.generic_sorter(opts),
    })
  end)
  return true
end

I think this is way more useful for extension developers.

@fdschmidt93
Copy link
Member

fdschmidt93 commented Dec 3, 2021

There are good reasons why you dont want to get all lines and filter with fzf.

I generally entirely agree. For completeness, I was referring to my use cases for what could be done with the PR :) consequently, I left it as is, because there's some hurdles to get it 'universally' right.

(As in, I cared about fzf over rg input, but when preferred I just blasphemously use fzf.vim for that, I'm afraid much simpler 😅)

@kylo252
Copy link
Contributor Author

kylo252 commented Dec 4, 2021

I think this is way more useful for extension developers.

Not arguing with that part, I just figured this can be an alternative in the meantime.

You might also be forgetting that this is not strictly about ripgrep, as it can also be used for example to implement workflows related to LSP that require some pre-processing, see #713, #1042 or even #642.

@Conni2461
Copy link
Member

This requires further discussion with tj. But he is on vacation, so this PR is on from our side. I'll think more about it

@Conni2461
Copy link
Member

Can you give an example of your interface. Like how you chain builtin commands

@kylo252
Copy link
Contributor Author

kylo252 commented Dec 10, 2021

Can you give an example of your interface. Like how you chain builtin commands

Sure thing, here's the full function that I used in the demo. It implements three different examples:

  • fuzzy_filter_results: works by chaining grep_string and assuming you have fzf as a sorter
  • dynamic_filetype: works by modifying the vimgrep_args passed to ripgrep
  • dynamic_filetype_skip: same as the above but with a --type-not flag instead

What would make this pretty clean is If I can figure out how to pass the opts to make these actions a bit more independent, but I think that requires adding it as a parameter to attach_mappings(). So far now, I have to wrap them internally.

chained_live_grep snippet
--- remember to point it to the new `run_builtin()`

function M.chained_live_grep(opts)
  local conf = require("telescope.config").values

  opts = opts or themes.get_ivy {}
  builtin.live_grep(vim.tbl_deep_extend("force", {
    prompt_title = "Search",
    attach_mappings = function(prompt_bufnr, map)
      local fuzzy_filter_results = function()
        local query = action_state.get_current_line()
        if not query then
          print "no matching results"
          return
        end
        vim.fn.histadd("search", query)
        opts.search = query
        opts.prompt_title = "Filter"
        opts.prompt_prefix = "/" .. query .. " >> "
        opts.next_picker = "grep_string"
        custom_actions.run_builtin(prompt_bufnr, opts)
      end

      local dynamic_filetype = function()
        local entry = action_state.get_selected_entry()
        local onlytype = vim.fn.fnamemodify(entry.filename, ":e")
        opts.vimgrep_arguments = vim.deepcopy(conf.vimgrep_arguments)
        opts.prompt_prefix = opts.prompt_prefix or "*." .. onlytype .. " >> "
        opts.prompt_title = "Scoped Results"
        vim.list_extend(opts.vimgrep_arguments, { "--type=" .. onlytype })

        opts.next_picker = "live_grep"
        custom_actions.run_builtin(prompt_bufnr, opts)
      end

      local dynamic_filetype_skip = function()
        local entry = action_state.get_selected_entry()
        local skiptype = vim.fn.fnamemodify(entry.filename, ":e")
        opts.vimgrep_arguments = vim.deepcopy(conf.vimgrep_arguments)
        opts.prompt_prefix = opts.prompt_prefix or "!*." .. skiptype .. " >> "
        opts.prompt_title = "Scoped Results"
        vim.list_extend(opts.vimgrep_arguments, { "--type-not=" .. skiptype })

        opts.next_picker = "live_grep"
        custom_actions.run_builtin(prompt_bufnr, opts)
      end

      -- local reset_search = function()
      --   opts.next_picker = "live_grep"
      --   opts.prompt_prefix = opts.prompt_prefix or " >> "
      --   custom_actions.run_builtin(prompt_bufnr, opts)
      -- end

      map("i", "<C-space>", fuzzy_filter_results)
      map("n", "<C-space>", fuzzy_filter_results)
      map("i", "<C-b>", dynamic_filetype)
      map("n", "<C-b>", dynamic_filetype)
      map("i", "<M-b>", dynamic_filetype_skip)
      map("n", "<M-b>", dynamic_filetype_skip)
      -- map("i", "<M-r>", reset_search)
      -- map("n", "<M-r>", reset_search)
      return true
    end,
  }, opts))
end

vim.cmd [[ command! ChainedLiveGrep :lua require("core.telescope.custom-finders").chained_live_grep()<CR> ]]

return M

@Conni2461
Copy link
Member

Whats the difference to doing this on latest master? You can just source the file and it should open chained_live_grep

local conf = require("telescope.config").values
local themes = require("telescope.themes")
local builtin = require("telescope.builtin")
local action_state = require("telescope.actions.state")

local function chained_live_grep(opts)
  opts = opts or themes.get_ivy {}
  builtin.live_grep(vim.tbl_deep_extend("force", {
    prompt_title = "Search",
    attach_mappings = function(_, map)
      local fuzzy_filter_results = function()
        local query = action_state.get_current_line()
        if not query then
          print "no matching results"
          return
        end
        opts.search = query
        opts.prompt_title = "Filter"
        opts.prompt_prefix = "/" .. query .. " >> "
        builtin.grep_string(opts)
      end

      local dynamic_filetype = function()
        local entry = action_state.get_selected_entry()
        local onlytype = vim.fn.fnamemodify(entry.filename, ":e")
        opts.vimgrep_arguments = vim.deepcopy(conf.vimgrep_arguments)
        opts.prompt_prefix = opts.prompt_prefix or "*." .. onlytype .. " >> "
        opts.prompt_title = "Scoped Results"
        vim.list_extend(opts.vimgrep_arguments, { "--type=" .. onlytype })
        builtin.live_grep(opts)
      end

      local dynamic_filetype_skip = function()
        local entry = action_state.get_selected_entry()
        local skiptype = vim.fn.fnamemodify(entry.filename, ":e")
        opts.vimgrep_arguments = vim.deepcopy(conf.vimgrep_arguments)
        opts.prompt_prefix = opts.prompt_prefix or "!*." .. skiptype .. " >> "
        opts.prompt_title = "Scoped Results"
        vim.list_extend(opts.vimgrep_arguments, { "--type-not=" .. skiptype })
        builtin.live_grep(opts)
      end

      map("i", "<C-space>", fuzzy_filter_results)
      map("n", "<C-space>", fuzzy_filter_results)
      map("i", "<C-b>", dynamic_filetype)
      map("n", "<C-b>", dynamic_filetype)
      map("i", "<M-b>", dynamic_filetype_skip)
      map("n", "<M-b>", dynamic_filetype_skip)
      return true
    end,
  }, opts))
end

chained_live_grep()

@kylo252
Copy link
Contributor Author

kylo252 commented Dec 10, 2021

I used to use that method, but there used to a problem when trying to invoke the builtin like so, is that you had to manually switch to insert-mode, see here: #1385 (comment)

Otherwise, I think that using the builtin-action helps reduce boilerplate code to do the same thing, with the added benefit of the entry_cb as well.

@Conni2461
Copy link
Member

insert-mode

Yes i know that this is an issue if you close the picker with actions.close but you dont even need to do that because if you just open a new picker while one is already open we close and open it correctly.

Also i dont see the reduction of boilerplate code. we go from

        opts.search = query
        opts.prompt_title = "Filter"
        opts.prompt_prefix = "/" .. query .. " >> "
        builtin.grep_string(opts)

to

        opts.search = query
        opts.prompt_title = "Filter"
        opts.prompt_prefix = "/" .. query .. " >> "
        opts.next_picker = "grep_string"
        custom_actions.run_builtin(prompt_bufnr, opts)

And the entry_cb is also kinda useless just do it before calling builtin.*.

I am sorry if i sound mean or so but i dont see how this is useful. all i see is that we go from

actions.select_default:replace(actions.run_builtin)

to

      actions.select_default:replace(function(prompt_bufnr)
        local selection = action_state.get_selected_entry()
        if not selection then
          print "[telescope] Nothing currently selected"
          return
        end
        opts.next_picker = selection.text
        actions.run_builtin(prompt_bufnr, opts)
      end)

And that we have a deepcopy of opts for some reason.

@kylo252
Copy link
Contributor Author

kylo252 commented Dec 11, 2021

Also i dont see the reduction of boilerplate code.
And that we have a deepcopy of opts for some reason.

I had assumed that this was necessary.

To re-iterate, just in case it wasn't obvious, what you're suggesting was literally my starting point, so I don't have anything against it, especially if that normal! A is no longer necessary.

I am sorry if i sound mean or so but i dont see how this is useful.

Do you mean chaining pickers in general or this specific change? You should try running this on master to see why this is useful.

:Telescope builtin theme=get_ivy

@Conni2461
Copy link
Member

Conni2461 commented Dec 11, 2021

Do you mean chaining pickers in general or this specific change? You should try running this on master to see why this is useful.
:Telescope builtin theme=get_ivy

Okay this one is busted because you dont pass the original opts into the actual call in run_builtin, but this doesnt mean we need to deepcopy them. This bug should be fixed. I would accept a PR. (But i am currently thinking we should move run_builtin out of action into a unnamed function closure (callback) because currently it has no purpose outside of builtin.builtin and i'd like to keep it that way)

Try to run the thing i provided you. I think its more clear and the same amount of code:

      local fuzzy_filter_results = function()
        local query = action_state.get_current_line()
        if not query then
          print "no matching results"
          return
        end
        opts.search = query
        opts.prompt_title = "Filter"
        opts.prompt_prefix = "/" .. query .. " >> "
        builtin.grep_string(opts)
      end

@kylo252
Copy link
Contributor Author

kylo252 commented Dec 11, 2021

I feel like I didn't get my point across, let me try to sum it up:

  1. I have no problems with your snippet because it's literally how I had to do it with the addition of adding vim.cmd[[normal! A]] because action._close is weird [*].
  2. I changed to using the run_builtin only because it was there and is able to call action._close correctly. The only thing missing was re-using the opts, which I added as an additional arg.
  3. I don't remember exactly why I added the deepcopy, I think I was trying to be clever about avoiding redrawing and needed to retain a clean copy of the opts.

I would accept a PR

In that case, this is actually the PR to fix that, since it's not adding anything else. You can always remove the deepcopy if it's a performance hit.

But i am currently thinking we should move run_builtin out of action into a unnamed function closure (callback) because currently it has no purpose outside of builtin.builtin and i'd like to keep it that way

If I understood it correctly, this might be superseded, or at very least achievable, by #1143. What do you think?


[*]: I had to know what was causing the problem and what the solution was, I finally found it 😄
see: https://github.com/nvim-telescope/telescope.nvim/pull/1554/files#diff-9ac2063a221b713185ddb6df3157cf01edf3b12894c2ae984e14250bf16ebd89R1143

@Conni2461
Copy link
Member

I thought we have fixed: [*] already months ago, maybe we only talked about it 😬

Okay here is what we do. I dont like this solution because i think it makes things harder. You have to know that you have to set opts.next_picker = "grep_string" before calling run_builtin which is super confusing.

We need to fix that opts arent passed in the builtin you that is run. I would accept something in the line like this (please open a new PR):

diff --git a/lua/telescope/actions/init.lua b/lua/telescope/actions/init.lua
index 7dfd61b..fc6bc81 100644
--- a/lua/telescope/actions/init.lua
+++ b/lua/telescope/actions/init.lua
@@ -336,26 +336,6 @@ actions.paste_register = function(prompt_bufnr)
   end
 end
 
-actions.run_builtin = function(prompt_bufnr)
-  local selection = action_state.get_selected_entry()
-  if selection == nil then
-    print "[telescope] Nothing currently selected"
-    return
-  end
-
-  actions._close(prompt_bufnr, true)
-  if string.match(selection.text, " : ") then
-    -- Call appropriate function from extensions
-    local split_string = vim.split(selection.text, " : ")
-    local ext = split_string[1]
-    local func = split_string[2]
-    require("telescope").extensions[ext][func]()
-  else
-    -- Call appropriate telescope builtin
-    require("telescope.builtin")[selection.text]()
-  end
-end
-
 actions.insert_symbol = function(prompt_bufnr)
   local symbol = action_state.get_selected_entry().value[1]
   actions.close(prompt_bufnr)
diff --git a/lua/telescope/builtin/internal.lua b/lua/telescope/builtin/internal.lua
index a868bc6..0c46f7f 100644
--- a/lua/telescope/builtin/internal.lua
+++ b/lua/telescope/builtin/internal.lua
@@ -81,7 +81,25 @@ internal.builtin = function(opts)
     previewer = previewers.builtin.new(opts),
     sorter = conf.generic_sorter(opts),
     attach_mappings = function(_)
-      actions.select_default:replace(actions.run_builtin)
+      actions.select_default:replace(function(prompt_bufnr)
+        local selection = action_state.get_selected_entry()
+        if selection == nil then
+          print "[telescope] Nothing currently selected"
+          return
+        end
+
+        actions._close(prompt_bufnr, true)
+        if string.match(selection.text, " : ") then
+          -- Call appropriate function from extensions
+          local split_string = vim.split(selection.text, " : ")
+          local ext = split_string[1]
+          local func = split_string[2]
+          require("telescope").extensions[ext][func](opts)
+        else
+          -- Call appropriate telescope builtin
+          require("telescope.builtin")[selection.text](opts)
+        end
+      end)
       return true
     end,
   }):find()

You can just use something like this for LunarVim and the chained_live_grep you are working on

      local fuzzy_filter_results = function()
        local query = action_state.get_current_line()
        if not query then
          print "no matching results"
          return
        end
        opts.search = query
        opts.prompt_title = "Filter"
        opts.prompt_prefix = "/" .. query .. " >> "
        builtin.grep_string(opts)
      end

And i would mentor a good pipe implementation / interface like i mentioned here: #1544 (comment)

Sorry but i dont see the point in this PR and it would be super surprising to me if tj would disagree with me here.

@kylo252
Copy link
Contributor Author

kylo252 commented Dec 11, 2021

We need to fix that opts arent passed in the builtin you that is run. I would accept something in the line like this (please open a new PR):

I don't really care about :Telescope builtin, I would have said to remove it altogether if it wasn't for the discoverability nature that it provides. Otherwise, the action has a confusing name, since it can run extensions as well.

You can just use something like this for LunarVim and the chained_live_grep you are working on

None of ths is really in LunarVim. It has been in my personal config for a while now, practically even before #1115, see our first discussion about this: https://matrix.to/#/!cxlVvaAjYkBpQTKumW:gitter.im/$Nikg-wq5o8nWVod7b7JXV6dCaDEiVmzjATpePNW5O2k?via=gitter.im&via=matrix.org&via=salt-rock-lamp.ems.host

So it's not a big deal for me either way, I just wanted to share this since it's been requested a lot as you can see.

Sorry but i dont see the point in this PR and it would be super surprising to me if tj would disagree with me here.

Fair enough, it's probably not a good idea to try to extend something that's kinda monolithic in the first place. Also, thankfully, my problem is already solved in #1143 😄

And i would mentor a good pipe implementation / interface like i mentioned here: #1544 (comment)

It would be good to open a ticket about this, with a concise set of features/constraints that you'd like to see. We can also have a discussion about that over on Matrix if you'd like.


Also regex is a thing that live_grep can do and grep_string on all lines, etc...

P.s. regex already works ootb, but I'm not sure if that's what you meant.

@kylo252 kylo252 closed this Dec 11, 2021
@kylo252 kylo252 deleted the flexible-run-builtin branch December 11, 2021 13:54
@Conni2461
Copy link
Member

Also regex is a thing that live_grep can do and grep_string on all lines, etc...
P.s. regex already works ootb, but I'm not sure if that's what you meant.

I was making an argument for live_grep and against grep_string on all lines (search on empty word). Missing a not there.

Ill open an issue, but you can write me on matrix if you need help

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

Successfully merging this pull request may close these issues.

Feature: Pickers chain Setting up fuzzy live grep
3 participants