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

plugins/telescope: support non-builtin keymaps #1544

Merged
merged 2 commits into from
May 28, 2024

Conversation

MattSturgeon
Copy link
Member

@MattSturgeon MattSturgeon commented May 20, 2024

Use the :Telescope command instead of relying directly on
require("telescope.builtin").

To quote upstream docs:

All telescope.nvim functions are wrapped in vim commands for easy access, tab completions and setting options.

Fixes #1493

@MattSturgeon MattSturgeon self-assigned this May 20, 2024
@MattSturgeon
Copy link
Member Author

MattSturgeon commented May 20, 2024

<cmd> mappings are not echod, so the keymapsSilent option now has no effect and could be deprecated.

@MattSturgeon MattSturgeon marked this pull request as ready for review May 21, 2024 03:39
Copy link
Member

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

<cmd> mappings are not echod, so the keymapsSilent option now has no effect and could be deprecated.

Let's do that in this PR then.

@MattSturgeon
Copy link
Member Author

I think there's a design decision to make here.

Using using the :Telescope command is simpler, since both builtin and extension pickers are directly available, and options can also be passed to the picker as part of the command.

The viml api can be called using commands like: :Telescope find_files hidden=true layout_config={"prompt_position":"top"}

This involves setting options using an = and using viml syntax for lists and dictionaries when the corresponding lua function requires a table.

However :h telescope.command does point out some limitations:

The viml api is more indirect, as first the command must be parsed to the
relevant lua equivalent, which brings some limitations.

One limitation of the viml api is that there can be no spaces in any of the
options.

For example, if you want to use the cwd option for find_files to
specify that you only want to search within the folder /foo bar/subfolder/
you could not do that using the viml api, as the path name contains a space.

It's also unclear whether all third-party extensions will expose their picker functions via the :Telescope command...


That said, this is already more powerful than the current approach, since the current telescope keymap wrapper doesn't allow passing any option arguments to the picker function anyway. Nor does it allow using third-party (extension) pickers.

If we decide we don't like the :Telescope approach, the only other way I can think of to (directly) support third-party picker functions is by adding an option along the lines of keymap.extension = mkNullOrOption str "If action is a third-party extension function, this should be the name of the extension.". I think this is more cumbersome from a UX perspective, though.


We could also augment either the old or new system by allowing action to be a raw string. This could be used as the body of a lua function where we've already defined some local variables (e.g. telescope, builtins, extensions). This way the user can use the lua API to call telescope functions.

@MattSturgeon MattSturgeon force-pushed the telescope_keymap branch 3 times, most recently from cbb2236 to d2b47ad Compare May 24, 2024 23:43
@MattSturgeon
Copy link
Member Author

@GaetanLepage could you re-review when you have chance?

Use the `:Telescope` command instead of relying directly on
`require("telescope.builtin")`.
Copy link
Member

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

Nice ! LGTM.

@MattSturgeon MattSturgeon merged commit 56d39f5 into nix-community:main May 28, 2024
50 of 55 checks passed
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.

[BUG] keymaps: mapping telescope's undo extension breaks custom keymaps
3 participants