`Picker:find()` : provide plugin support for mappings

[hinell]

Description

https://github.com/nvim-telescope/telescope.nvim/blob/6258d50b09f9ae087317e392efe7c05a7323492d/lua/telescope/pickers.lua#L564-L566

Per docs it should be possible to configure plugins via global require("telescope").setup(...) config, but Picker:find (used by extensions) somehow ignores local opts.mappings and acesses global mappings instead. The mappings provided to extension via require("telescope").setup(...) call aren't automatically used inside extension initialization until implemented via on_attach. The following won't work:

require("telescope").setup({
   extensions = {
      pluginName = {
         mappings = {
            i = { [...] = actions. ... }
         }
      }
   }
})

I think this issue should be addresssed by providing opts to the Picker:find

Telescope version / branch / rev

6258d50b09f9ae087317e392efe7c05a7323492d

[jamestrew]

If I understand the issue correctly, I think it's very similar to https://github.com/nvim-telescope/telescope.nvim/issues/2470.

Just to be sure, do you mean extensions when you say "plugins"? If so, you can already have custom mapping support for extensions by binding the mappings to the attach_mappings option and passing it to the picker. You can see an example here: https://github.com/nvim-telescope/telescope-file-browser.nvim/blob/e0fcb12702ad0d2873544a31730f9aaef04fd032/lua/telescope/_extensions/file_browser.lua#L68.

This function is the main picker function exported by the extension telescope-file-browser. When you call :Telescope file_browser, this function gathers up all the options set in the setup function as well as any passed to the function call itself, applies a theme if needed, and then uses attach_mappings to bind all the mappings before calling the picker.

When this line runs in picker:find(), self.attach_mappings should already have all the mappings for your extension.

mappings.apply_keymap(prompt_bufnr, self.attach_mappings, config.values.mappings)

@lougreenwood, this might be a better explanation of how you can integrate mappings into extensions.

[hinell]

@jamestrew I'm aware of attach_mappings. I've used it for mapping config. The docs then should saying explicitly, that you can't configure mappings for extensions via require("telescope").setup({ extensions = }) call. Otherwise it's confusing. Please make explanations like that above in docs, instead of issues. The first thing users see are docs, nobody got time to dig up issues really. Thanks!

[jamestrew]

Just to be clear, you can configure mappings for extensions via the setup function. Referring to telescope-file-browser again:

require("telescope").setup {
  extensions = {
    file_browser = {
      theme = "ivy",
      -- disables netrw and use telescope-file-browser in its place
      hijack_netrw = true,
      mappings = {
        ["i"] = {
          -- your custom insert mode mappings
        },
        ["n"] = {
          -- your custom normal mode mappings
        },
      },
    },
  },
}

I'll try to describe this in the docs if I have some time.

[hinell]

@jamestrew I think I just told you above that you can't configure mappings via extensions =, please read. Many extensions simply ignore these options. telescope-recent-files for example doesn't accept mappings.

I think it's bad design approach to configure external extensions in a separate config provided to telescope setup function.

[jamestrew]

My point is, extension authors CAN make their extension mappings configurable via extensions = and there are extensions which do implement this. The documentation probably can use some improvements to support this but ultimately it's the extension author's responsibility to do so.

I don't know what your expectation is from this issue. You want a way for extension authors to support custom mappings via extensions = and I'm telling you this already exists. You should open an issue with the extension if you want them to support it or better yet open a PR yourself.

[hinell]

what your expectation is from this issue.

1. I think it's not Telescope's responsibility to provide cofingure to extensions so it should be ultimately deprecated.
2. Until deprecated, the docs should explicitly state that extensions = { mappings = ... } depends on plugin implementation and may not work.
3. Given what was said above: an example of extension implementing mappings should be provided

[lougreenwood]

Just to bring this back on topic...

@hinell whilst I agree, that telescope extension config doesn't belong in Telescope setup (and some extensions can already handle this for us, e.g https://github.com/danielfalk/smart-open.nvim/issues/15#issuecomment-1445030080), please move that to a new issue to keep this on topic 🙏.

@jamestrew I'm not familiar with the implementation details of Telescope, but it seems reasonable (given the use case of Telescope) that extensions must be forced to implement specific config, for example setting mappings when using whatever API Telescope provides to build an extension.

IMO relying on documentation and the knowledge & good will of extension authors to have a decent UX doesn't seem to address the issue. Having to manually update every Telescope extension to get this feature rather than every telescope extension by default being required to allow mapping override.

For example, if every extension published it's config to a registry in telescope config and then telescope using that registry for mapping lookups. That's just one possible implementation method idea - I'm sure there are much better techniques to handle this.

[lougreenwood]

One thing I've been doing in the past few days is to assign a custom function to my <CR> telescope mapping and then trying to infer what extension is open to do the correct thing - but the issue here is that there doesn't seem to be any consistent interface for identifying a given picker/extension by an ID, but rather I need to infer what it might be given the payload that is received - which is fragile.

[hinell]

I've updated the issue description, please checkout.

please move that to a new issue to keep this on topic

Well I think the mappings initialization for extensions done via require("telescope").setup(...) should either be dropped, or it should be enforced so that extensions develoeprs implement this feature. Otherwise docs are misleading and Issues like this one are risen. This should be addressed eventually.

[lougreenwood]

@hinell I'm sorry, I didn't realise this was a different issue! I thought this was the issue I opened since it was my first interaction in this repo, so when I started to receive notifications in my email I assumed it was a conversation in my issue, that's why I mentioned about keeping it on topic 🤦.

Sorry to tell you what to do in your own issue - my mistake 😳🙈!

Well I think the mappings initialization for extensions done via require("telescope").setup(...) should either be dropped,

IMO, all initialisation is better in that extensions own setup() - then let telescope handle just it's own config 👍

[hinell]

IMO, all initialisation is better in that extensions own setup()

@lougreenwood Thumbed up your issue on that #2470. This current issue is actually related to the #2470