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: set defaults for each picker in telescope setup #883

Merged
merged 4 commits into from
Jun 9, 2021

Conversation

Conni2461
Copy link
Member

Close #582

Its so long that i wrote telescope code. LUL

I don't like the idea of custom pickers, the one that @tami5 proposed. I think for that people should just continue to go with own wrapper functions. But this helps clean up other things. Like this e.g.

local actions = require('telescope.actions')

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

I am not sure if this is the best way to implement this feature

@kkharji
Copy link
Member

kkharji commented Jun 1, 2021

THANKS SO MUCH CONNI. I'm really disappointed 😢 that I haven't gotten around and implemented this. but awesome work, I wouldn't have done it better

for mapping I suggest we simplify it a bit

mappings = {
  ['n|<C-d>'] = 
}

I already implemented this in some other projects, so I can share a code snippet here

This better because it save a lot of space. what do u think

@Conni2461
Copy link
Member Author

for mapping I suggest we simplify it a bit

I would not do that for a number of reasons

  1. Simpler for one mapping, but for that you can also just do this i = { ["<c-d>"] = actions.delete_buffer, } but for multiple mappings it introduces additional ways on how the configuration could break, that would not happen with a table. So for each mapping you have a chance to introduce a typo or forget the mode etc.
  2. This would be a now mapping interface which would differ from the current one used in the defaults tbl
  3. So if we would do this style in both we would break backwards compatibility
  4. If we add it additionally we introduce possible ways how the configuration code could break and just add confusion to why we support multiple ways
  5. I think we gain nothing from changing it. So nay from my side

@dhruvmanila
Copy link
Contributor

Wow, looking forward to configuring in this way!

Maybe we could provide a field to define the keymap for opening the picker? The user can also provide the default keymap options to pass. This would simplify defining the dozen key bindings for all the pickers ;)

require("telescope").setup {
  pickers = {
    buffers = {
      open_with = "<C-b>",
      ...
      }
    }
  }
}

Or, instead of mapping each picker individually, a table specifically for mapping all the pickers, similar to that of gitsigns:

require("telescope").setup {
  picker_keymaps = {
    noremap = true,
    silent = true,
    buffers = "<C-b>", 
    -- or ["<C-b>"] = "buffers",
    -- or ["<C-b>"] = require('telescope.builtin').buffers,
    -- or ["<C-b>"] = "<cmd>lua require('telescope.builtin').buffers()<cr>",
    }
  }
}

@Conni2461 Conni2461 force-pushed the handle_pickers_in_config branch 2 times, most recently from 95e2b6f to 01be067 Compare June 1, 2021 19:03
@Conni2461 Conni2461 requested a review from tjdevries June 2, 2021 18:01
@tjdevries
Copy link
Member

Oh, this gave me an idea.

What if we also added the ability for mappings right hand side to just be the key from actions as a shorthand, for very simple setup for people not used to Lua.

So instead of actions.delete_buffer, it would instead be "delete_buffer" .

Thoughts?

@Conni2461 Conni2461 force-pushed the handle_pickers_in_config branch 2 times, most recently from f27b453 to dfa967d Compare June 3, 2021 09:54
@@ -191,7 +191,11 @@ mappings.apply_keymap = function(prompt_bufnr, attach_mappings, buffer_keymap)
local key_bind_internal = a.nvim_replace_termcodes(key_bind, true, true, true)
if not applied_mappings[mode][key_bind_internal] then
applied_mappings[mode][key_bind_internal] = true
telescope_map(prompt_bufnr, mode, key_bind, key_func)
if type(key_func) == "string" then
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should just actually do this check above at the top of the function, inside of the map function we create on line 104 -- that way any time someone uses a string, it defaults to searching actions.

@Conni2461 Conni2461 force-pushed the handle_pickers_in_config branch from 216dc89 to 169342c Compare June 3, 2021 14:14
@dhruvmanila
Copy link
Contributor

dhruvmanila commented Jun 4, 2021

I have a question: I know that concatenating actions works because of __add but will it be the same for concatenating action strings?

mappings = {
  i = {
    -- This works
    ["<c-q>"] = actions.send_to_qflist + actions.open_qflist,
    -- But this won't work
    -- E5108: Error executing lua [string ":lua"]:1: attempt to perform arithmetic on a string value
    ["<c-q>"] = "send_to_qflist" + "open_qflist",
  }
}

Maybe for multiple actions, we can ask for a table of strings like so: { "send_to_qflist", "open_qflist" } and the actions will be executed in that order.

@Conni2461
Copy link
Member Author

Conni2461 commented Jun 4, 2021

Okay the current interface that is implemented is:

mappings = {
  i = {
    -- current
    ["<c-d>"] = actions.smart_send_to_qflist + ....,
    -- same as
    ["<c-d>"] = { actions.smart_send_to_qflist + ...., type = "action" }, 
    -- future opts = { nowait = true } in table for all tables. See #890 

    -- To make user configuration more simple for people not familiar with lua
    ["<c-d>"] = "smart_send_to_qflist",
    -- is basicall this:
    ["<c-d>"] = { "smart_send_to_qflist", type = "action_key" },

    -- for viml
    ["<c-d>"] = { '<cmd>:echo "Hello"<CR>', type = "command" },
  }
}

Plus for actions should still work. But i am not sure if we should do "send_to_qflist" + "open_qflist", i will talk with tj about it

@tjdevries
Copy link
Member

Wow, looking forward to configuring in this way!

Maybe we could provide a field to define the keymap for opening the picker? The user can also provide the default keymap options to pass. This would simplify defining the dozen key bindings for all the pickers ;)

require("telescope").setup {
  pickers = {
    buffers = {
      open_with = "<C-b>",
      ...
      }
    }
  }
}

Or, instead of mapping each picker individually, a table specifically for mapping all the pickers, similar to that of gitsigns:

require("telescope").setup {
  picker_keymaps = {
    noremap = true,
    silent = true,
    buffers = "<C-b>", 
    -- or ["<C-b>"] = "buffers",
    -- or ["<C-b>"] = require('telescope.builtin').buffers,
    -- or ["<C-b>"] = "<cmd>lua require('telescope.builtin').buffers()<cr>",
    }
  }
}

I think we could do something like this, but it's out of scope for this PR. In fact, what I'd prefer to do is probably wait til I get a little farther on something like https://github.com/tjdevries/vimterface.nvim and consider hooking up to that plugin rather than re-creating it all here.

@Conni2461 Conni2461 force-pushed the handle_pickers_in_config branch from 2267fc1 to 13d5efe Compare June 9, 2021 17:46
@Conni2461 Conni2461 merged commit 618e0e6 into nvim-telescope:master Jun 9, 2021
@Conni2461 Conni2461 deleted the handle_pickers_in_config branch June 9, 2021 17:51
@Shatur
Copy link

Shatur commented Jun 9, 2021

@Conni2461, thank you very much, I really missed this feature.

Can we add the ability to specify a default theme?

After this PR we can do the following:

require("telescope").setup {
  pickers = {
    buffers = {
      theme = "dropdown",
    }
  }
}

But it would be nice to be able to also specify a theme for each buffer:

require("telescope").setup {
  defaults = {
      theme = "dropdown",
  }
}

@Conni2461 Conni2461 mentioned this pull request Jun 10, 2021
4 tasks
@Conni2461
Copy link
Member Author

@Shatur Currently Layout Configuration is completely being reworked here #823
This could be part of that PR as well. I suggested it at least

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.

Enable users to setup configuration for individual pickers
5 participants