Closed oddlama closed 10 months ago
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 git init
on fzf.fish, I'm leaning towards not merging this. HOWEVER, if you are willing to get on a call to teach me the code AND stay on as a maintainer to maintain this particular feature, then I would be open to it.
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
git init
on fzf.fish, I'm leaning towards not merging this. HOWEVER, if you are willing to get on a call to teach me the code AND stay on as a maintainer to maintain this particular feature, then I would be open to it.
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 :) And best of luck with parenting and your job!
I don't think this feature would require much maintenance (as long as no new features/config options are added
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.
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.
True.
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.
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 :)
~Installed this PR but the completions don't trigger at all. When I run bind, \t _fzf_complete doesn't even show up. Can you look into it please?~ It was my fault: I installed the plugin from GitHub not the local version I had.
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:
set: given 1 indexes but 0 values
math: Error: Too few arguments
'2 +'
^
cut: illegal option -- -
tons of those errors no matter which command's options I'm trying to complete. Are you on Linux? I'm on Mac.
tons of those errors no matter which command's options I'm trying to complete. Are you on Linux? I'm on Mac.
cut: illegal option -- -
Yep I'm on NixOS. cut
is the only required external utility (apart from fzf of course), but it is really essential. I already tried to not use it, but there's nothing native in fish that is fast enough to replace it.
I was wondering what other fish fzf plugins did, but it seems like they all rely on cut
in the same way. The difference seems to be the --zero-terminated
, which is not supported on the decades version that ships with mac (still based on the old NetBSD cut I presume). It is a crucial component that makes all of this robust, but fortunately according to this stackexchange post the solution is to just install coreutils
to get the proper coreutils and not the dumbed-down version. Can't check though, I don't own any macs.
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... That said, I got it working and it's quite lovely.
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 https://github.com/PatrickF1/fzf.fish/pull/293#issuecomment-1454581678, 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 https://github.com/PatrickF1/fzf.fish/pull/293#issuecomment-1454863174, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPAJEDRI3YVJB6E4RRBNCTW2OPSBANCNFSM6AAAAAAU4BCA5A . You are receiving this because you commented.Message ID: <PatrickF1/fzf. @.***>
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.
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 l<TAB>
), it's not because the script is slow, but unfortunately because fish's completion system is just hellishly slow (just time the raw command: time complete --escape --do-complete l
).
Nice @oddlama! So changing it from cut to string
calls didn't slow it down significantly?
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
So changing it from cut to string calls didn't slow it down significantly?
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. On my main computer I didn't even see any lag with tab completion.
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.
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?
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 complete --escape
also fixed a lot of things for fish's internal pager. Sorry for that, should've checked more carefully.
(But don't worry, there still are a lot of bugs in fish's completion system that we inherit š¢ , e.g. try to do touch aaa\nbbb
then ls a<TAB>
works but dd if=<TAB>
doesn't - I guess at least we also inherit the fixes automatically, when fish decides to fix that)
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?
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.
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.
Thanks š
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.
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. Other than that it would probably simpler and faster if you just force push any of your desired style changes directly, without going through me. Those won't affect the functionality either way and then you can just do what fits your style best.
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.
And in general just enough knowledge about fish etc. to hurt myself.
Just today I stumbled over https://github.com/PatrickF1/fzf.fish and later this PR, both together do scratch my itches perfectly.
Finding out that the just learned delta
integration for git log
did not work in this fork, I just rebased the patches and trying out the results as stored in https://github.com/passionsfrucht/fzf.fish.
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. I plan to use this rebased version for now until I find something blocking and plan to let you know then.
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
\n
and others, where the internal completion would fail\0
in 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~
,$HOME
and 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:
=
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/completed
I 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