Aloxaf / fzf-tab

Replace zsh's default completion selection menu with fzf!
MIT License
3.41k stars 96 forks source link

Add 'complete-common-prefix' option. #384

Closed jochenwierum closed 9 months ago

jochenwierum commented 1 year ago

This option automatically completes to a common prefix of all options instead of starting fzf if such a prefix exists.I had the same problem as FunctionalHacker in #8

Let's assume there is the following directory:

# ls -1
123a
123b
123c
234d

When typing ls 2<TAB> I expect that my prompt changes to "ls 234d". When typing ls 123<TAB> I expect to see fzf with three alternatives (123a-123c). When typing ls <TAB> I expect to see fzf with all files. However, when typing ls 1<TAB> I don't expect fzf to appear at all. Instead, I like to see a completion to ls 123 first.

I was able to hack this behavior into fzf-tab. It can be disabled by setting the zstyle parameter complete-common-prefix to false. However, the solution feels hacky and I'm not an expert on zsh or its completion system. While it works for my scenarios, I have no idea if it breaks something else. So please review this PR very very carefully!

cohml commented 9 months ago

TL;DR

Nice work, but I've think I've identified a few issues with this PR.

Hopefully they can be resolved because I too would kill for the target behavior to be implemented into fzf-tab!

Details

After trying out your branch, I've found two problems. @Aloxaf I encourage you to weigh in if you can.

Problem 1: Slashes after directories

This is somewhat of a corner case, but does degrade the UX, so should still probably be dealt with.

Given a directory containing the following

$ ls
foo     # directory
foo.sh  # file

f<TAB> cause the following autocompletion (at least for me):

$ ls foo/

The appended slash effectively prevents fzf-tab from finding foo.sh, so the second <TAB> just descends into foo/ instead of opening fzf-tab.

Problem 2: Cross-platform variation

I've found that the behavior varies on different platforms. I attempted to reproduce what you stated in the PR's description:

when typing ls 1<TAB> I don't expect fzf to appear at all. Instead, I like to see a completion to ls 123 first.

To that end, after setting up the file tree that you described

$ cd $(mktemp -d) && touch 123{a..c} 234d

I tested the latest commit (2470f1e) on the following systems:

$ zsh --version
zsh 5.8.1 (x86_64-amazon-linux-gnu)
$ zsh --version
zsh 5.9 (x86_64-apple-darwin23.0)

On Amazon Linux, it worked exactly as advertised, with $ ls 1<TAB> completing the prompt to $ ls 123, and then 123<TAB> opening fzf-tab.

But on MacOS, $ ls 1<TAB> opened fzf-tab and simply pre-filled the fzf query string with 123. This is still an improvement, but it is not the behavior you described.

I'm not a zsh expert, but after comparing line-by-line the setopt output on both systems, I see no differences. The only difference apart from architecture is zsh version, which may or may not explain this discrepancy.

jochenwierum commented 9 months ago

Thank you very much for your detailed feedback!

I observed problem no 1, too. But I was not aware that my PR introduced this behavior. I was able to fix it by removing the -f and -W flags from compadd. I'm very uncertain if it is the right way to to this. I updated my branch accordingly.

After I "fixed" the first problem, I tried to reproduce problem no 2. I have a debian oldstable (zsh 5.8) and an ubuntu linux (zsh 5.9). With both, I was not able to reproduce your problem. I guess, my zsh knowledge is not good enough :)

cohml commented 9 months ago

@jochenwierum And thank you for the prompt attention! I've just tested the changes you forced pushed (using commit 4b37ea7), and both problems reported seem to have been resolved. Great work!

However, in testing I did notice two other regressions. Not sure if they existed before as I don't think I tested for this specific case.

If all files in a directory contain the same prefix - which is definitely something everyone's bound to encounter from time to time, e.g. log files - tab completion actually "eats" the path. I'm curious to see if you're able to reproduce this.

Run this command to set up the test:

TMPDIR=$(mktemp -dp.) && touch $TMPDIR/prefix.{1..5}

You should now see the following:

$ ls -1 $TMPDIR
prefix.1
prefix.2
prefix.3
prefix.4
prefix.5

Now try this:

ls $TMPDIR/<TAB>

For me, nothing happens. I expected the prompt to complete to ls $TMPDIR/prefix. (obviously $TMPDIR will also expand). So that's the first regression.

Next, and much stranger, start entering the common prefix and re-attempt the tab completion:

ls $TMPDIR/p<TAB>

For me, <TAB> causes the entire $TMPDIR/p argument to disappear. It just gets mysteriously eaten, returning my prompt to simply ls.

This same behavior happens for any of

ls $TMPDIR/p<TAB>
ls $TMPDIR/pr<TAB>
ls $TMPDIR/pre<TAB>
ls $TMPDIR/pref<TAB>
ls $TMPDIR/prefi<TAB>
ls $TMPDIR/prefix<TAB>

But strangely, when I do

ls $TMPDIR/prefix.<TAB>

fzf-tab opens as you'd expect. Perhaps that's because at this point, the common prefix has been exhausted such that the next character will result in options being filtered out.

In my mind, these observations suggest that whatever these regressions stem from, it resides in the "all options share a common prefix" branch of the conditional tree.

I wish I could assist further e.g., patching your code, but my grasp of zsh is even weaker than yours (which seems quite good btw!).

Aloxaf commented 9 months ago

I observed problem no 1, too. But I was not aware that my PR introduced this behavior. I was able to fix it by removing the -f and -W flags from compadd. I'm very uncertain if it is the right way to to this. I updated my branch accordingly.

Removing those flags unconditionally may not be a good idea, it causes the bug addressed in https://github.com/Aloxaf/fzf-tab/pull/384#issuecomment-1955143830.

Aloxaf commented 9 months ago

I think this may be because -W accepts a parameter, so they must be removed together. However, removing only -f seems work well.

jochenwierum commented 9 months ago

Thanks for pointing me to the tests. Indeed, it is enough to remove -f.

I added a test for completion with prefix-completion and without prefix-completion. They both work now.

But there is still a problem with nested directories, as the tests point out.

mkdir -p abc/def/hij abc/dfe/hij
./a/d/h<TAB>

My normal zsh behaviour is, that the prompt changes to ./abc/d<CURSOR>/h, pressing again suggests "def" and "dfe".

With my common-prefix implementation, the prompt changes to ./abc/d/h<CURSOR> (the prefix is completed, but the cursor is at the wrong position). Pressing again does not help. So after I invoke compadd, I need to place the cursor correctly. Any ideas how to do this?

Disabling the common-prefix implementation keeps ./a/d/h in the prompt and suggests "def" and "dfe". It then completes the full path correctly.

I added tests for both implementations (they are red) and pushed my current version.

cohml commented 9 months ago

@jochenwierum Following up -

Your most recent patch resolves all issues for me in my own testing (except I guess the nested dir thing you mentioned - I didn't even know a/d/h<TAB> would work!).

I love it and hope you can get this merged soon. Keep it up!

Aloxaf commented 9 months ago

I finally managed to get it to work. Could you please try #413 ?

Aloxaf commented 9 months ago

Closed by #413. Thanks for your contribution anyway!

jochenwierum commented 9 months ago

Great work! It does exactly what I tried to achieve! Thank you very much for your work!