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

Improve Fish's completions #212

Merged
merged 3 commits into from
Sep 18, 2021
Merged

Improve Fish's completions #212

merged 3 commits into from
Sep 18, 2021

Conversation

kidonng
Copy link
Contributor

@kidonng kidonng commented May 16, 2021

This PR brings two improvements to Fish's completions, when completing {{cmd}}:

  • Let Fish only complete directories and excludes files

  • If there is no match in the current directory, Fish will query zoxide with current token and serve them as completions:

    image

    This is approximately the same as using zoxide query -i <current token>, but more integrated with Fish.

Let me know if you have any suggestion over these two changes.

@ajeetdsouza
Copy link
Owner

Thanks! Two small issues I had when trying this:

  • If I type in z foo bar and hit Tab, I only get results for bar (foo is ignored).
  • If I type in z foo bar and use the autocomplete, it will complete only bar, leaving me with z foo /home/bar. Since this contains multiple arguments, it doesn't cd correctly.

@kidonng
Copy link
Contributor Author

kidonng commented May 17, 2021

  • If I type in z foo bar and hit Tab, I only get results for bar (foo is ignored).

Only the token under the cursor is searched, see reasons below.

  • If I type in z foo bar and use the autocomplete, it will complete only bar, leaving me with z foo /home/bar. Since this contains multiple arguments, it doesn't cd correctly.

That's just how tab completion works. You can't expect it to change anything else than the current token. It's also why it doesn't take any other token into consideration, which just won't make sense.

If people do want to search with multiple tokens, they should use zoxide query -i instead.

@ajeetdsouza
Copy link
Owner

I understand that, but IMO completions should never result in a command that is invalid, which is what's happening here.

Btw, merging this PR might take a while - I want to try a couple of different solutions to see what works best.

@kidonng
Copy link
Contributor Author

kidonng commented May 19, 2021

but IMO completions should never result in a command that is invalid

You can easily reproduce the same issue with regular cd, and I don't think anyone have an issue with that.

To achieve what you expect, every completion needs to implement a full-fledged parser, which is obviously not practical.

Technically we can prevent providing completion if the user is completing a second argument, for this issue only though. But IMO that's just unnecessary.

@ajeetdsouza
Copy link
Owner

ajeetdsouza commented Sep 3, 2021

@kidonng if you have time, do take a look at #257. I've only implemented Bash completions at the moment, but I'd love to know what you think:

  • Completions only work when there is one word (therefore completions are always valid).
  • By default, it uses the same completions as cd.
  • If the query ends with **, it opens up fzf and does an interactive search (similar to fzf's builtin completions).

If it works well, we can hopefully write completions for other shells in line with this.

@ajeetdsouza ajeetdsouza force-pushed the main branch 2 times, most recently from 866e54b to bc89589 Compare September 9, 2021 03:51
@ajeetdsouza
Copy link
Owner

Hey @kidonng, I've updated the PR. This is what the new completion function does:

  • It completes directories by default.
  • If there is more than one argument, or if the last argument ends in **, it makes an interactive query via fzf.
  • Early exit from interactive query removes the ** if present.

I'd appreciate any thoughts on this.

@ajeetdsouza ajeetdsouza merged commit 2f2f588 into ajeetdsouza:main Sep 18, 2021
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.

2 participants