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

Make it possible to disable Ctrl+T / Alt+C / completions #3678

Merged
merged 6 commits into from
Mar 17, 2024

Conversation

elibarzilay
Copy link
Contributor

This makes it possible to skip one of the above key bindings or completions by setting a variable to an empty string. For example,

FZF_CTRL_T_COMMAND= FZF_ALT_C_COMMAND= FZF_COMPLETION_TRIGGER= \
  eval "$(fzf --zsh)"

would leave only the history search. Makes it more convenient then producing the files and changing them myself to not include what I don't want.

This should be working, but for completeness it should also include at least some explaining comment at the top (since there's no docs about these vars?) and something equivalent for bash/fish.

This makes it possible to skip one of the above key bindings or
completions by setting a variable to an empty string. For example,

    FZF_CTRL_T_COMMAND= FZF_ALT_C_COMMAND= FZF_COMPLETION_TRIGGER= \
      eval "$(fzf --zsh)"

would leave only the history search. Makes it more convenient then
producing the files and changing them myself to not include what I don't
want.

This should be working, but for completeness it should also include at
least some explaining comment at the top (since there's no docs about
these vars?) and something equivalent for bash/fish.
shell/completion.zsh Outdated Show resolved Hide resolved
shell/completion.zsh Outdated Show resolved Hide resolved
@junegunn
Copy link
Owner

something equivalent for bash/fish

Can you update those files as well?

@elibarzilay
Copy link
Contributor Author

something equivalent for bash/fish

Can you update those files as well?

Bash would be easy, but I don't have experience with fish so it'd need to wait until I have time to do it properly...

@junegunn
Copy link
Owner

Well, I wasn't too happy with it anyway, since it looks a bit random to just hook on one variable per disable-able feature.

Yeah, I kind of felt the same. But the idea of disabling a key binding if its COMMAND variable is intentionally set to an empty string doesn't seem too far-fetched and it's the simplest approach at hand. So I'm going to accept it.

The limitation of this approach though is that it doesn't address the need of some users to bind the feature to a different key. But it's not easy to come up with a solution that works consistently with all 3 shells because they all use different key expressions.

I'll take over this PR from here and finish it up (bash, fish, and documentation). Thanks.

@junegunn junegunn changed the base branch from master to devel March 16, 2024 14:29
@junegunn junegunn changed the base branch from devel to master March 16, 2024 14:58
@junegunn junegunn merged commit 88f4c16 into junegunn:master Mar 17, 2024
5 checks passed
@junegunn
Copy link
Owner

Merged, thanks!

@elibarzilay
Copy link
Contributor Author

@junegunn Huge thanks for taking it over!

Some replies that are the least I owe you:

  • Might be nice to replace the *COMMAND by some *KEY so you can both disable it or change the key that is used.
  • Yes, I'm using --zsh to get both files. The sed thing is kind of lame as a way to disable loading one file, but yeah, it's hard to find some elegant variable.
  • Sidenote, for the purists that dislike jumping onto sed for whatever, you can use the shell instead. It's also possible to avoid leaving the code in a temporary variable like this:
    eval "$(x=$(fzf --zsh); echo "${x//"### completion"*"completion.zsh ###"}")"
    
    This should work at least for zsh / bash.

@junegunn
Copy link
Owner

junegunn commented Mar 18, 2024

So what was your motivation behind this? Which feature did you want to opt-out? I decided to make --*sh print both script files because I figured most of the users would want both.

  • Might be nice to replace the *COMMAND by some *KEY so you can both disable it or change the key that is used.

That would be ideal, but I wanted the variables to work consistently across three different shells to minimize the confusion, but it's not easy to achieve that with the approach because all three shells use different key expressions. ^T, \C-t, \ct.

@bluz71
Copy link

bluz71 commented Mar 19, 2024

I decided to make --*sh print both script files because I figured most of the users would want both.

Maybe I am not "most users" but I certainly wish fine grain control over the scripts. We are not quite there yet (from what I can see).

In the old mechanism there existed separate completion.bash and keybindings.bash scripts, and I only ever sourced the later (keybindings) and never the former (completion). Completion slowed my SHELL startup too much (about 20ms) whilst keybindings only added a couple ms. I don't need ** support.

In the new mechanism eval "$(fzf --bash)" I can not figure out how to not load completion.

Ideally I would like something like:

FZF_COMPLETION= eval "$(fzf --bash)"

I do want Ctrl-r, Ctrl-t, Alt-c but I do not want fuzzy ** completion.

@bluz71
Copy link

bluz71 commented Mar 19, 2024

I just saw FZF_COMPLETION_TRIGGER= eval "$(fzf --bash)", my bad. However, this has poor startup performance:

% time FZF_COMPLETION_TRIGGER= eval "$(fzf --bash)"
real	0m0.029s

Compared with old-school . $HOMEBREW_PREFIX/opt/fzf/shell/key-bindings.bash:

time . $HOMEBREW_PREFIX/opt/fzf/shell/key-bindings.bash
real	0m0.004s

29ms vs 4ms.

It would be nice to have true opt-out of Bash fzf ** completion.

@junegunn
Copy link
Owner

junegunn commented Mar 19, 2024

@bluz71

I just saw FZF_COMPLETION_TRIGGER= eval "$(fzf --bash)"

That part was not merged.

I can not figure out how to not load completion

eval "$(fzf --bash | sed '/### completion/,$d')"

29ms vs 4ms.

You don't have to use --bash, you can still use the old method if you prefer it. The files are still and will always be included in the package.

@bluz71
Copy link

bluz71 commented Mar 19, 2024

Thanks @junegunn, eval "$(fzf --bash | sed '/### completion/,$d')" works.

I assume that the the scripts on-disk will eventually go away?

I am satisfied.

@bluz71
Copy link

bluz71 commented Mar 19, 2024

Oh, the scripts will always be available, if so, then I will stick with those.

All good, sorry for the diversion.

@junegunn
Copy link
Owner

No problem, thanks for sharing your case.

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.

3 participants