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

fix(zsh): handle backtick trigger edge case #4090

Merged
merged 9 commits into from
Nov 14, 2024

Conversation

LangLangBart
Copy link
Contributor

@LangLangBart LangLangBart commented Nov 12, 2024

Description

If a single backtick is used as a trigger, the words array fails to populate
with the command elements.

The idea here is to temporarily alter the LBUFFER before calling
__fzf_extract_command by escaping the backtick.

The idea is to move the CURSOR before the trigger.

This approach seems a bit hacky, which is why this PR is marked as a draft.

Alternative A

Create a list of invalid triggers, since other characters have also been found
to cause the same issue:

Testing covered only ASCII values 32-126; others were excluded for simplicity.

Symbol Common Name ASCII Occurrences in Public Git Repos
& Ampersand 38 1
; Semicolon 59 ~25
[ Left Bracket 91 1
` Back-tick 96 ~20
| Pipe 124 0

If the user attempts to use one, display a message using zle -M [string] to
indicate the error. This places the string below the command line, prompting the
user to choose a different trigger character.

Alternative B

Fall back to the old extraction method if the words array is empty and the
LBUFFER is not empty, with the downside that it would only work for the first
command.

--- a/shell/completion.zsh
+++ b/shell/completion.zsh
@@ -123,9 +123,20 @@ __fzf_comprun() {
 # Extract the name of the command. e.g. ls; foo=1 ssh **<tab>
 __fzf_extract_command() {
+  local token tokens
   setopt localoptions noksh_arrays
-  # Control completion with the "compstate" parameter, insert and list noting
+  # Control completion with the "compstate" parameter, insert and list nothing
   compstate[insert]=
   compstate[list]=
   cmd_word="${words[1]}"
+  [[ -n $cmd_word ]] && return
+  tokens=(${(z)LBUFFER})
+  for token in $tokens; do
+    token=${(Q)token}
+    if [[ "$token" =~ [[:alnum:]] && ! "$token" =~ "=" ]]; then
+      cmd_word="$token"
+      return
+    fi
+  done
+  cmd_word="${tokens[1]}"
 }

@@ -341,5 +352,4 @@ fzf-completion() {
     # Make the 'cmd_word' global
     zle __fzf_extract_command || :
-    [[ -z "$cmd_word" ]] && return

     [ -z "$trigger"      ] && prefix=${tokens[-1]} || prefix=${tokens[-1]:0:-${#trigger}}

Alternative B might be the best option. If a user encounters an issue like the
one described in #1992, simply suggest using a different trigger?

@jefferickson
Copy link

I'd advocate for Alternative B as I've used the backtick as my trigger for 6+ years. The key is right next to the on an American layout and quite convenient.

Thank you!

It's just important that the cursor is in the 'command' part before the trigger; the actual position is not important.
@LangLangBart
Copy link
Contributor Author

For the current version, the CURSOR is moved before the trigger, as the
words array likely interprets any of the listed characters in the table above
as the start of a new command.

The approach would still extract the correct command.

More testing of these changes is needed.

An error occured when the trigger was assigned to '('
@LangLangBart LangLangBart marked this pull request as ready for review November 12, 2024 20:27
@LangLangBart
Copy link
Contributor Author

I'd advocate for Alternative B

That was also my intention, but temporarily moving the cursor before calling the
__fzf_extract_command completion widget seems to work well and ensures that
the user still gets the correct list of matches displayed in commands like below:

ls ; kill -9 `[TAB]

Please test the latest patch. The zsh version affects many users, and it's
important to avoid releasing with errors. Feedback will help ensure the fix
works well. Thanks!

source <(curl -fsSL https://raw.githubusercontent.com/LangLangBart/fzf/refs/heads/zsh_backtick/shell/completion.zsh)

@jefferickson
Copy link

Please test the latest patch.

The patch works for me and my backtick trigger! Thank you!

@junegunn
Copy link
Owner

junegunn commented Nov 14, 2024

I'm testing the latest revision, and I'm seeing this warning message.
image

Turns out I have setopt WARN_CREATE_GLOBAL in my .zshrc. Can we fix it? Or am I supposed to remove the setting from my config? From a confused bash user.

Having said that, the fix works as expected.

@LangLangBart
Copy link
Contributor Author

Turns out I have setopt WARN_CREATE_GLOBAL in my .zshrc. Can we fix it? Or am I supposed to remove the setting from my config? From a confused bash user.

Thanks for testing. I will add typeset -g cmd_word.

man zshoptions | less --pattern 'WARN_CREATE_GLOBAL'
WARN_CREATE_GLOBAL
  Print a warning message when a global parameter is created in  a
  function  by an assignment or in math context.  This often indi-
  cates that a parameter has  not  been  declared  local  when  it
  should  have  been.   Parameters explicitly declared global from
  within a function using typeset -g do not cause a warning.  Note
  that  there  is no warning when a local parameter is assigned to
  in a nested function, which may also indicate an error.
# see 'zshbuiltins'  for 'typeset'
man zshbuiltins

@junegunn
Copy link
Owner

Thanks, it all looks good to me, is there anything else you want to address?

@LangLangBart
Copy link
Contributor Author

Thanks, it all looks good to me, is there anything else you want to address?

I am finished. During testing, I found no other issues. I hope fzf users won't find any either.

The bugs (missing enhancement) reported from my zsh patches weren't created intentionally, and I am sorry you had to deal with so many releases.

@junegunn
Copy link
Owner

No problem at all, I really appreciate your help.

@junegunn junegunn merged commit 03d6ba7 into junegunn:master Nov 14, 2024
5 checks passed
@junegunn
Copy link
Owner

Merged thanks!

@jefferickson
Copy link

Thank you @LangLangBart ! Really appreciate the fast turn around and response to my report.

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.

0.56.1/2 breaks my FZF_COMPLETION_TRIGGER
3 participants