A proper implementation of fzf-tab #293
Conversation
- don't add spaces after completing directories - don't add space after arguments like --color= - add -- (end of options) before completing filenames starting with - - proper initial query with prefix detection - omit common path prefix when completing paths
oddlama
changed the title
|
Hi @oddlama! I'm currently swamped but didn't want you to leave you hanging for too long so I'll give you my preliminary thoughts while not having throughly understood the code. First off, I'm incredibly humbled and grateful you'd submit such a big contribution to fzf.fish. You seem to have really done the research on the best technical solution and know what I'm looking for--good documented code, robustness, reliability, even wrote out detailed explanations so I can understand it faster. Just because of how busy in life I've become (new parent, new job, bad health circumstances) and how I've already fulfilled the goals I started out with when I ran |
No worries, I can totally understand that. I just already had done the work and figured it cannot hurt to contribute it. At this point though I think I have to politely decline your kind offer, since I am not sure whether I will stick with fish over a longer timespan. While fish is awesome in many ways, zsh's completions system is just an order of magnitude more powerful - and (un)fortunately I really accommodated myself to those features. While I don't think this feature would require much maintenance (as long as no new features/config options are added), I fear I wouldn't do it justice in case I left fish again. I'm happy regardless of whether this is merged or not, but I really think a lot of people would like this feature - so let me propose an alternative:
Oh and regardless of the above, I'm of course open to giving you a walk-through of the code if you like :) |
That's what previous contributors told me but bugs or weird edge cases that appear to be bugs w/o a deep understanding of the implementation always crop up. Okay I understand as well from your perspective. I like your recommendation. It just seems unlikely to work so I'll probably either merge this or close this after I have enough time to study the code more deeply. I really hope you can make this your own plugin (I'd install it!) but OSS is masochism so I don't recommend it to anyone in good conscience. |
True.
This month I'm a little busy with other stuff too but around March to April I think I'll can have another look at it. If it hasn't been picked up by anyone until then I'll see if I can quickly package it as a separate plugin. Thanks for the insightful conversation :) |
| set --query _flag_directory && set key_sequences[1] "$_flag_directory" | ||
| set --query _flag_git_log && set key_sequences[2] "$_flag_git_log" | ||
| set --query _flag_git_status && set key_sequences[3] "$_flag_git_status" | ||
| set --query _flag_history && set key_sequences[4] "$_flag_history" | ||
| set --query _flag_processes && set key_sequences[5] "$_flag_processes" | ||
| set --query _flag_variables && set key_sequences[6] "$_flag_variables" | ||
| set --query _flag_complete && set key_sequences[7] "$_flag_complete" |
There was a problem hiding this comment.
$_flag_complete doesn't get set unless you modify options_spec above to add something like 'complete=?'
There was a problem hiding this comment.
Thanks, I've added a new option accordingly
|
|
|
Thanks for the fixes! Oh boy, I was afraid of this: I think my cut on my machine is different than the cut on your machine: tons of those errors no matter which command's options I'm trying to complete. Are you on Linux? I'm on Mac. |
Yep I'm on NixOS. I was wondering what other fish fzf plugins did, but it seems like they all rely on |
|
Oh noooo that's really unfortunate. Not only do I not want to introduce a new dependency, the main problem I foresee is that if we add in Search Completions, either
Unfortunately, with fisher, I have very limited support for communicating with the users. The best I can do is insert something on the plugin update and how they see it, which historically has produced mixed results. Maybe this does need to be a new plugin... |
|
How can I use this? I downloaded changes from this PR to right places and it is for searching files (Ctrl+Alt+F)? |
|
Down the changes, run fisher install, and it should be triggered by tab
…On Fri, Mar 3, 2023 at 11:04 PM roland-5 ***@***.***> wrote:
How can I use this? I downloaded changes from this PR to right places and
it is for searching files (Ctrl+Alt+F)?
—
Reply to this email directly, view it on GitHub
<#293 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAPAJEDHGYHMO4BT2RXNE73W2LSPZANCNFSM6AAAAAAU4BCA5A>
.
You are receiving this because you commented.Message ID: <PatrickF1/fzf.
***@***.***>
|
|
Thank you. Damn, this looks nice. If problem is with Mac users only, so maybe make this as a plugin? I will use this very often from now on and I think there would be more people happy with this. It seems better option than not merging this. |
|
If I had more time, I’d look into seeing if the cut logic can be emulated
using the fish string combination and fzf’s field expression functionality.
Can someone who is reading this please take a look? I and many others will
be eternally grateful!
…On Sat, Mar 4, 2023 at 12:18 PM roland-5 ***@***.***> wrote:
Thank you. Damn, this looks nice. If problem is with Mac users only, so
maybe make this as a plugin? I will use this very often from now on and I
think there would be more people happy with this. It seems better option
than not merging this.
—
Reply to this email directly, view it on GitHub
<#293 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAPAJEDRI3YVJB6E4RRBNCTW2OPSBANCNFSM6AAAAAAU4BCA5A>
.
You are receiving this because you commented.Message ID: <PatrickF1/fzf.
***@***.***>
|
I was wrong before, I just had an idea how to realize this. Will push an updated version soon. And just a reminder if completions seem very slow (such as |
|
Nice @oddlama! So changing it from cut to |
|
How does this implementation differs from https://github.com/gazorby/fifc ? |
|
I just wanted to say this is the most positive and mutually understanding/respectful interaction in OSS I've seen in a while. Much love to both of you <3 |
No not noticeably. The main delay seems to come from fish itself, so nothing we can influence. Oh and I just noticed that this will bump the minimum required fish version to 3.4.0 (released 12.03.2022) from 3.2.0, because of complete --escape. Just another thing to be aware of. |
|
I didn't notice any difference in time respond if I do tab without any letter (where everything will show off) on my Orange Pi 3 with Debian Sid. It take little time, but the same amount (for OPi3) in previous and actual implementation. |
|
Hi again, I honestly don't have my life together now and it's going to be a while before I do. It doesn't help that the code is very complex so it's taking me a long time to digest. That said, I'm leaning towards merging this and would appreciate your patience. Since I'm only digesting a bit of your code at a time, do you mind if I leave piecemeal feedback on your PR? I know it's annoying that you don't get it all at once to address in one fell swoop--going back and forth and sometimes previous feedback will become irrelevant--but that's the best I can do. |
|
Sure, take all the time you need to sort out your personal stuff first! I'll just address your comments as you go along. |
| function _fzf_search_completions --description "Shell completion using fzf" | ||
| # Produce a list of completions from which to choose | ||
| # and do nothing if there is nothing to select from | ||
| set -l cmd (commandline -co) (commandline -t) |
There was a problem hiding this comment.
could you use the long version of these flags please? --cut-at-cursor --tokenize, --current-token. Btw, how come you need the second (commandline -t)?
There was a problem hiding this comment.
Good suggestion, will do. Regarding your question, consider ls -l --color=<TAB>
commandline --cut-at-cursor --tokenizewill yieldlsand-l(only tokens before the cursor)commandline --current-tokenyields--color=(the current token until the cursor)
|
You mention in the PR "with less bugs than fish's internal pager". What bugs does the fish internal completion pager suffer from that this doesn't? |
Now that you mention it, i think I was wrong. Apparently this is not true anymore since fish 3.4, before which the internal pager was not able to complete filenames with newlines (or other weird special characters) in them, which does work now. Since this PR raises the requirement for fish to 3.4 it would be very unfair to say it has less bugs, instead I just failed to make the connection that the introduction of (But don't worry, there still are a lot of bugs in fish's completion system that we inherit 😢 , e.g. try to do |
oddlama
changed the title
|
Ok so correctness wise, you think this PR is on par with fish's internal pager, and the improvement on top of it is purely having fzf as the interface? |
| function _fzf_search_completions --description "Shell completion using fzf" | ||
| # Produce a list of completions from which to choose | ||
| # and do nothing if there is nothing to select from | ||
| set -l cmd (commandline --cut-at-cursor --tokenize) (commandline --current-token) |
There was a problem hiding this comment.
could you explain why you do this thing of breaking up the command into two parts (before cursor and the current token)? is the reason because you want to remove everything after the current token?
There was a problem hiding this comment.
Yes, for generating completions we only want to consider what comes before the cursor. The reason why keep it in an array using the --tokenize flag is that this still allows the code later to access the individual tokens, instead of coercing it to a single string. This is currently not needed technically, but the commented out part below might use it in the future to eliminate completions that are already present (eg. passing --color twice makes no sense).
So it's not actually two parts, it's just concatenating two arrays as commandline doesn't offer this in a single command.
There was a problem hiding this comment.
I see thanks for explaining.
This is currently not needed technically, but the commented out part below might use it in the future to eliminate completions that are already present (eg. passing --color twice makes no sense).
For the sake of a lower-risk and faster MVP, can you update this to just use normal complete --do-complete? I also suspect we'll never build this out because even though for most commands the same option twice doesn't make sense, there will be a few in the wild that breaks that assumption.
There was a problem hiding this comment.
Scratch my suggestion about doing the completion of the entire commandline vs considering what comes before the cursor. We definitely want to only complete what comes before the cursor rather than at the end of the command line.
| # A prefix that will be prepended before each selected completion | ||
| set -l each_prefix "" | ||
|
|
||
| # Only skip fzf if there is a single completion and it starts with the expected completion prefix. |
There was a problem hiding this comment.
WDYM "starts with the expected completion prefix"? why would complete --do-complete ever return a completion the user wouldn't want?
There was a problem hiding this comment.
Sometimes the shell transforms or eliminates quotes, variables, ...
If that happens, I don't want this to silently change the commandline and instead prompt the user with fzf so any change is previewed and can be cancelled if needed.
Consider ls foo"bar" could be completed as foobar, but the user might have intended to place quotes there to e.g. later insert content with spaces more easily. I often do that and it's annoying if deletes the quotes. If there for some reason is only one completion, this shouldn't just replace that.
There was a problem hiding this comment.
Ah okay this is consistent with how fish currently operates: if only one completion, it will auto-complete while preserving existing quotes and backslashes. Makes sense, good thinking!
| @@ -4,7 +4,7 @@ function fzf_configure_bindings --description "Installs the default key bindings | |||
| # no need to install bindings if not in interactive mode or running tests | |||
| status is-interactive || test "$CI" = true; or return | |||
|
|
|||
| set options_spec h/help 'directory=?' 'git_log=?' 'git_status=?' 'history=?' 'processes=?' 'variables=?' | |||
| set options_spec h/help 'directory=?' 'git_log=?' 'git_status=?' 'history=?' 'processes=?' 'variables=?' 'completions=?' | |||
There was a problem hiding this comment.
reminder to self (or I suppose you could do it too oddlama), need to update the configure bindings help function.
PatrickF1
force-pushed
the
main
branch
2 times, most recently
from
19e9830 to
cab67cc
Compare
|
|
||
| # Only skip fzf if there is a single completion and it starts with the expected completion prefix. | ||
| # Otherwise, the completion originates from a match in the description which the user might | ||
| # want to review first (e.g. man ima<TAB> might match `feh`, an image viewer) |
There was a problem hiding this comment.
I don't think this is possible? complete --do-complete "fzf_configure_bindings change" and complete --do-complete "fzf_configure_bindings -change" both return nothing even though change matches the description of several fzf_configure_bindings options
There was a problem hiding this comment.
The feh example was a real issue I encountered. Not sure if there are other fields used here, but this is definitely necessary to prevent matching non-real matches. Not sure how fish works here internally.
There was a problem hiding this comment.
Odd! I'll look into this more.
| set --append descriptions (string split --fields 2 --max 1 --right \t -- $completions[$i] ; or echo "") | ||
| end | ||
|
|
||
| # Find the common prefix of all completions. Yes this seems to be a hard |
There was a problem hiding this comment.
Oh shoot, I just realized that by overriding the existing tab functionality, we actually need to fill in a lot more than just searching completions, including filling up to the common prefix. Hmm now I'm on the fence again about implementing this UNLESS we use a different key binding and shed this feature, so the user knows when they call the function, they are purely doing completions search and nothing else. I really do not want to be in the business of replicating what fish already does, only to supplement it.
There was a problem hiding this comment.
Do with this PR what you want :)
| case 1 | ||
| # User accepted without selecting anything, thus we will | ||
| # use just the current query | ||
| set results $fzf_output[1] |
There was a problem hiding this comment.
This comment is better described as "Nothing matched the user's query so just use their query".
EDIT: on a second thought, this is not the behavior of any of the other search commands in this plugin, which is to exit without doing anything if you user selects nothing. Not only is that, I would argue, less surprising the the user who intentionally selected nothing at all, it's also important to have consistent UX with all the other search commands. So I think we should remove this.
| # that expects a parameter (like --color= or dd if=). The same logic applies here. | ||
| set -l first_char (string sub -l 1 -- $results[1]) | ||
| set -l last_char (string sub -s -1 -- $results[1]) | ||
| if test $last_char = =; or test $last_char = / |
There was a problem hiding this comment.
you can use -o instead of ; or test
| # Technically not always correct (the program may not accept --), but the alternative is worse | ||
| # and this will make the user notice it. | ||
| for r in $results | ||
| test - = (string sub -l 1 -- $r); and test -e $r; and set prefix "-- "; and break |
There was a problem hiding this comment.
can you make this into a normal if statement for readability, please?
also can use -a to merge the two tests
| # When a completion ends in a / we usually want to keep adding to that completion. | ||
| # This may be because it is a directory (or link to a directory), or just | ||
| # an option that takes a category/name tuple like `emerge app-shells/fish`. | ||
| # Also, completions like ~something are directories. |
There was a problem hiding this comment.
Home directories, such as ~patrick for example
There was a problem hiding this comment.
No, ~/something is the folder called something in the home directory of the currently logged-in user, while ~something is the home directory of the user named something
There was a problem hiding this comment.
Oh shoot I didn't know this syntax existed!
|
Hi @oddlama, finally finished my first read of the code (sorry I really had almost no free time until today) and I probably understand about 85% of it. Wow this is extremely thorough and it is truly a labor of love! Implementing completions and emulating current fish completion behavior is such a monumental undertaking. Kudos to your diligence and creativity making this work. I still foresee a 2-3 passes before we beta-release it (don't add it to the default bindings but announce it). Let me know if you don't want to work on this anymore. |
Thanks 😄
I'm unfortunately also a little short on time right now. If you have any major concerns I'm of course happy to address them later. |
|
Hi! Foremost, let me please thank you for the inspiring and wholesome conversation. Truly inspiring! Unfortunately, I share the same problem as you two do; not enough free time to be able to maintain the code. I just fixed the resulting conflicts in the most straightforward manner, and all of them made sense in my opinion. However, as I said, I am not fully aware of what I am doing in fish, so I may have introduced some bugs. Additionally, I'm not sure how to handle this, so it fits into your workflows, thus I just leave it as a link here and let you know about it. |
|
Thanks Steffen! I think it's clear I don't have enough time for this so what I'll do is I'll beta release this by not adding a key binding by default, and whoever wants can bind it themselves and play around with it. Once I'm confident all the bugs have been caught, then I'll release it. |
|
Eagerly awaiting this. Is there anything I can do to help? |
|
Dang, I'm sorry everyone. Long story short I'm not going to merge this and never will include this feature. I finally had a free night and gave it a good go for a couple of hours. What made me realize this feature is not a good fit:
My conviction is that searching completions is better left to the fish-shell devs as an built-in. Sorry for getting everyone's hopes up! |
This implements fzf-tab completion as robustly as reasonably possible with fish. All remaining bugs are directly related to bugs (or weird design decisions) in fish. Some edge cases are different from edge cases in fish's completion, but I did absolutely everything in my power to create the best possible implementation (that is still reasonably simple). As you already pointed out, existing plugins like https://github.com/jethrokuan/fzf were known to be very very buggy because they were implemented without watching out for correctness.
\t\nand others, where the internal completion would fail\0in any logic to prevent introducing new edge-casesFrom what I could gather from #190 and #217 this still seems to be a controversial topic here. I just felt like implementing it correctly wasn't too complicated and would actually be less buggy than the built-in completion. I'd love if you would reconsider having a look at this and then decide if it is worth to you or not. I think a lot more people would benefit from it when it can be integrated here as opposed to me creating a new plugin for it. It's fine if you decide not to merge it, I just hope someone might find it useful.
Feature overview
Generally, i tried mimicking Aloxaf's fzf plugin for zsh as much as possible, since it is the most mature fzf tab completion I could find. The main problem here being that zsh is just infinitely more capable when it comes to completions because it returns a lot of completion context, type and other information in a predictable manner, which are not available (or even existing) in fish. So all remaining issues all come from issues with fish that the internal completion also has. I've outlined known issues at the end.
Feature-wise, this means:
--color=touch --mistake; rm --mis<TAB>) will terminate options with--before inserting the file completion~,$HOMEand other variables are handled correctly without requiring expansionThis is a baseline implementation, so any advanced features or config options some user's would like are not included (e.g. path prefix completion without opening fzf). I think it is a reasonable starting point from where users can customize it themselves if needed. There's theoretically some room for customization (description color, using fzf's preview, ...) which are not considered at the moment.
Known Issues with fish's completion
There are some things that the internal completion can't do which we cannot fix:
=character fish-shell/fish-shell#5363: folders ending in=break fish's internal completion (It looks like an argument with a file to fish:this_folder_feels_like_an_argument=/)It's important to know that fish cannot give reliable context information on completions. Knowing whether a completion is a file or argument can only be determined via a heuristic. Effectively this means some features are currently not viable:
--nonexisting-argument=/file/will/be/completedI hope that with this PR we can give fzf-tab another chance.
EDIT: Seems like this PR violates your formatting. This is the first time I write fish, so I'd be glad if you could tell me how to make the code compliant :)
Oh and users coming from fzf-tab might want to additionally use
set fzf_complete_opts --cycle --reverse --height=50%to get the tab completion the way they are used to