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

refactor: simplify command extraction in zsh completion #4082

Merged
merged 4 commits into from
Nov 10, 2024

Conversation

LangLangBart
Copy link
Contributor

Description

The idea is to gain acces to the words array.

man 1 zshcompwid | less --pattern="^\s+words"
#   words  This  array  contains the words present on the command line currently being edited.

The great thing about this array is that the first element contains the name of
the command. The only downside is that the words array is available only in
user-defined completion widgets, which are created with zle -C, as opposed
to user-defined widgets created with zle -N.

In a command like ls; foo=bar kill **[TAB], words[1] will be kill.

To understand how the words array is constructed, one must read the zsh
source code1.

[…] the completion system has already done the hard job (and that's not an
overstatement, I can tell you) of deciding what makes up a word on the command
line, taking into account quoting and special characters. The array of words
is stored, unsurprisingly, in the array $words.[…]

Source: A User's Guide to the Z-Shell - Chapter 6: Completion2


Notes

Does that mean the user has to run autoload -Uz compinit && compinit in their
setup to use fzf's completion from now on?

NO.

The new zsh completion system basically relies on shell functions, and all
of them become available as soon as the user calls compinit. Since we don't
use any of these functions here, nothing in the setup changes.

The fzf completion system is designed to be quite minimal for a couple of
commands, like kill, and to be easily extended with a shell function.

Zsh's system has become more and more sophisticated, and in version 3.1.6 a
new completion system appeared which is supposed to do everything for you: you
simply call a function, compinit, from an initialization file, after which zsh
knows, for example, that gunzip should be followed by files ending in .gz. The
new system is based on shell functions, an added bonus since they are
extremely flexible and you already know the syntax. However, given the
complexity it's quite difficult to get started writing your own completions
now, and hard enough to know what to do to change the settings the way you
like.

Source: A User's Guide to the Z-Shell - Chapter 6: Completion2

There is a very nice debug hotkey one can press, control-X + ?, and it will
list the function calls in a debug file.

man 1 zshcompsys | less --pattern _complete_debug

In the debug example below, we actually see that zsh also relies on the
words array in their _set_command3 function to extract the command that
shall be completed, and this is where the entire idea for this patch came from.

command env -i HOME=$HOME TERM=$TERM USER=$USER PATH=$PATH PAGER=$PAGER zsh -f
autoload -Uz compinit && compinit

# Enter a command, for example 'ls' and press 'control-X + ?'
ls [^X?]
# Trace output left in /tmp/zsh58725ls1 (up-history to view)
# ...

less /tmp/zsh58725ls1 ;: 'ls '
# ...
#   +_normal:37> _set_command
#    +_set_command:6> local command
#    +_set_command:8> command=ls
# ...

Footnotes

  1. zsh/Code - Src/Zle/complete.c

  2. A User's Guide to the Z-Shell - Chapter 6: Completion, old and new 2

  3. zsh/Code - Completion/Base/Utility/_set_command

@LangLangBart
Copy link
Contributor Author

LangLangBart commented Nov 9, 2024

why the test failed

  2) Failure:
TestZsh#test_dir_completion [test/test_go.rb:3684]:
Expected: "cd /tmp/fzf-test/d55/"
  Actual: "cd /tmp/fzf-test/d55"

254 runs, 2627 assertions, 1 failures, 0 errors, 1 skips

The test seems to fail due to a zsh oddity, explained below:

As it turns out, if you attempt to use completion without first defining at
least one widget, zsh automatically loads zsh/compctl just so that something
sensible will happen. The illusion of predefined completion widgets is
maintained, but it really is an illusion.

Source: https://zsh.org/mla/workers/2003/msg00703.html

Running the Docker command below and entering cd /tmp followed by pressing
TAB will not add a / (slash).

docker run --rm --tty --interactive ubuntu:24.10 sh -uec '
apt-get update && apt-get install -y zsh
rm -rf /etc/zsh
echo "testing() { : }; zle -C testing .complete-word testing" > ~/.zshenv
/usr/bin/zsh'

Running the same command without the ~/.zshenv line will cause cd /tmp[TAB]
to add the / (slash).

Widgets can be listed with zle -l.


The following test has been added to prevent future issues:

# Check if at least one completion system (old or new) is active
if ! zmodload -e zsh/compctl || ! (( $+functions[compdef] )); then
  zmodload -i zsh/compctl
fi

@LangLangBart LangLangBart marked this pull request as ready for review November 9, 2024 13:51
@LangLangBart
Copy link
Contributor Author

LangLangBart commented Nov 9, 2024

The following test has been added to prevent future issues:

# Check if at least one completion system (old or new) is active
if ! zmodload -e zsh/compctl || ! (( $+functions[compdef] )); then
  zmodload -i zsh/compctl
fi

I would suggest adjusting the test_go.rb file instead of adding this if case
to the fzf-completion widget.

--- a/test/test_go.rb
+++ b/test/test_go.rb
@@ -3681,4 +3681,9 @@ module CompletionTest
     tmux.send_keys :BSpace, :BSpace, :BSpace
     tmux.until { |lines| assert_equal 'cd /tmp/fzf-test/d55', lines[-1] }
+    if instance_of?(TestZsh)
+      tmux.send_keys 'C-c'
+      tmux.send_keys 'zmodload -i zsh/compctl', :Enter
+      tmux.send_keys 'cd /tmp/fzf-test/d55'
+    end
     tmux.send_keys :Tab
     tmux.until { |lines| assert_equal 'cd /tmp/fzf-test/d55/', lines[-1] }

Mostly, I believe that there isn't an actual person with such a bizarre setup,
except when testing an edge case like this. (And for the use of functions, one
would need to add another check to see if the zsh/parameter module was loaded.)

Should I adjust the test file instead of the fzf-completion widget?


EDIT: Disregard the comment above, it is indeed a possible setup.

EDIT2: The command zmodload -i zsh/compctl should remain and be executed,
as the mere presence of zle -C … somehow hinders the dynamic loading of
zsh/compctl.

@junegunn
Copy link
Owner

Thanks! it works great as expected.

@junegunn junegunn merged commit 57c08d9 into junegunn:master Nov 10, 2024
5 checks passed
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Nov 22, 2024
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [junegunn/fzf](https://github.com/junegunn/fzf) | minor | `v0.55.0` -> `v0.56.3` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>junegunn/fzf (junegunn/fzf)</summary>

### [`v0.56.3`](https://github.com/junegunn/fzf/releases/tag/v0.56.3): 0.56.3

[Compare Source](junegunn/fzf@v0.56.2...v0.56.3)

-   Bug fixes in zsh scripts
    -   fix(zsh): handle backtick trigger edge case ([#&#8203;4090](junegunn/fzf#4090))
    -   revert(zsh): remove 'fc -RI' call in the history widget ([#&#8203;4093](junegunn/fzf#4093))
    -   Thanks to [@&#8203;LangLangBart](https://github.com/LangLangBart) for the contributions

### [`v0.56.2`](https://github.com/junegunn/fzf/releases/tag/v0.56.2): 0.56.2

[Compare Source](junegunn/fzf@v0.56.1...v0.56.2)

-   Bug fixes
    -   Fixed abnormal scrolling behavior when `--wrap` is set ([#&#8203;4083](junegunn/fzf#4083))
    -   \[zsh] Fixed warning message when `ksh_arrays` is set ([#&#8203;4084](junegunn/fzf#4084))

### [`v0.56.1`](https://github.com/junegunn/fzf/releases/tag/v0.56.1): 0.56.1

[Compare Source](junegunn/fzf@v0.56.0...v0.56.1)

-   Bug fixes and improvements
    -   Fixed a race condition which would cause fzf to present stale results after `reload` ([#&#8203;4070](junegunn/fzf#4070))
    -   `page-up` and `page-down` actions now work correctly with multi-line items ([#&#8203;4069](junegunn/fzf#4069))
    -   `{n}` is allowed in `SCROLL` expression in `--preview-window` ([#&#8203;4079](junegunn/fzf#4079))
    -   \[zsh] Fixed regression in history loading with shared option ([#&#8203;4071](junegunn/fzf#4071))
    -   \[zsh] Better command extraction in zsh completion ([#&#8203;4082](junegunn/fzf#4082))
-   Thanks to [@&#8203;LangLangBart](https://github.com/LangLangBart), [@&#8203;jaydee-coder](https://github.com/jaydee-coder), [@&#8203;alex-huff](https://github.com/alex-huff), and [@&#8203;vejkse](https://github.com/vejkse) for the contributions

### [`v0.56.0`](https://github.com/junegunn/fzf/releases/tag/v0.56.0): 0.56.0

[Compare Source](junegunn/fzf@v0.55.0...v0.56.0)

-   Added `--gap[=N]` option to display empty lines between items.
    -   This can be useful to visually separate adjacent multi-line items.
        ```sh
        ```

### All bash functions, highlighted

      declare -f | perl -0777 -pe 's/^}\n/}\0/gm' |
        bat --plain --language bash --color always |
        fzf --read0 --ansi --reverse --multi --highlight-line --gap
      ```
      <img width="855" alt="image" src="https://github.com/user-attachments/assets/b3d2eaf2-bf44-4e3a-8d7b-9878629dd9be">
    - Or just to make the list easier to read. For single-line items, you probably want to set `--color gutter:-1` as well to hide the gutter.
      ```sh
      fzf --info inline-right --gap --color gutter:-1
      ```
      <img width="855" alt="image" src="https://github.com/user-attachments/assets/113757a1-ccfd-42a6-b946-83533f408e69">

-   Added `noinfo` option to `--preview-window` to hide the scroll indicator in the preview window
-   Bug fixes
    -   Thanks to [@&#8203;LangLangBart](https://github.com/LangLangBart), [@&#8203;akinomyoga](https://github.com/akinomyoga), and [@&#8203;charlievieth](https://github.com/charlievieth) for fixing the bugs

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
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.

Completion in zsh is always based on first command
2 participants