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

Remove folder name prefix in cd tab autocompletion suggestions #8618

Open
AlexSWall opened this issue Jan 5, 2022 · 29 comments
Open

Remove folder name prefix in cd tab autocompletion suggestions #8618

AlexSWall opened this issue Jan 5, 2022 · 29 comments

Comments

@AlexSWall
Copy link

fish version. 3.3.1
OS. MacOS Monterey v12.1
Terminal. iTerm2 3.4.14
Behaviour persists on running sh -c 'env HOME=$(mktemp -d) fish'.

When typing cd and going into a directory, using tab completion prefixes the last directory in front of all results, sometimes truncated and prefixed by .... For example, bar below is needlessly prefixed.

Screenshot 2022-01-05 at 23 03 56

I'm finding this very inconvenient when, for example, trying to tab complete at ~/Documents/, which prefixes ...ocuments/ in front of every result. More than half of the text returned is useless. It looks something like the below (but worse).

Screenshot 2022-01-05 at 23 08 02

How can I have fish only list the directory names?

@krobelus krobelus added this to the fish-future milestone Jan 6, 2022
@krobelus
Copy link
Member

krobelus commented Jan 6, 2022

This has previously been requested in #7883 (and @thithib's #8617)

I think it's reasonable to special-case file completion here, because it's already special: we complete one path component at a time, not the entire token.
So I believe we could accept this inconsistency in the pager (custom completions like for git rm will still have the current pager).
This means that filtering (with Control-S) will only consider the basename, but that's a good thing (today you can even filter for the ellipsis character (), which seems like an accident).

This should probably be kept consistent with other completions of sub-tokens, like for grep --color= or dd if= (ok dd is an exception).

Sadly it's not so easy to do the same for completions that recursively call complete -C for file completions, because fish can't tell if these are files.
Those are not used very much, I think __fish_complete_suffix is the main one. (search for the __fish_command_without_completions hack)

I don't think we need a configuration option (in theory it's possible to write your own pager that parses complete -C)

@faho
Copy link
Member

faho commented Jan 6, 2022

I don't see how this helps. It's an inconsistency, for very little gain.

You can see for yourself - the screenshot does use "half" of the space for the prefix, but why does it matter? It's still on one line!

I'm not sure how the length of the ellipsized prefix is determined today and there might be room for improvement, but having this consistent across completions is nice. Having a marker "hey, this is where this differs, this is how it fits in" is nice.

This means that filtering (with Control-S) will only consider the basename, but that's a good thing (today you can even filter for the ellipsis character (…), which seems like an accident).

This would be easy enough to do in general - just don't search the prefix. Note this would be annoying e.g. for git commit hashes, because you might not want to see where it differs and only type from there.

@AlexSWall
Copy link
Author

AlexSWall commented Jan 6, 2022

So that I'm not creating a strawman of the problem, here's two screenshots, the first of fish, and the second of zsh, for something that roughly mirrors my Documents folder.

Screenshot 2022-01-06 at 11 10 04

Screenshot 2022-01-06 at 11 10 18

Hopefully it's not too controversial to say that the first is objectively worse than the latter here. There's much more clutter, I find it much harder to see what's going on, particularly at speed, and it's now multiple lines which makes doing a scan more annoying at speed. Imagine this but with 3x the number of folders.
Also, I think the case will be worse for those with dyslexia, which may be contributing for me here.

I'm happy for it to be, and think it'd be reasonable for it to be, a special case for path completion, and not hold for other options, and for this to be disabled by default but enabled via a config flag. Seems very reasonable to me.

That said, I'm not against a hack which allows me to set the colours such the initial text is barely visible and the actual contents are brightly coloured, though this is far from desirable. I'm also not against just making a fork of the repo with just this change which I maintain if it's not a difficult change and you don't want it in the main repo. I'm also not against doing the PR if you can give me suitable pointers to what to do.

I'd honestly just assumed there was a built-in way to change it and I couldn't for the life of me find it.
It's currently the only thing that I'd describe as simply inferior in fish to how bash and zsh have it, which is a shame as it's so fundamental to using a terminal.

@faho
Copy link
Member

faho commented Jan 6, 2022

Hopefully it's not too controversial to say that the first is objectively worse than the latter here

It is, I do not believe it is worse at all.

The first is consistent with every other completion, and it makes it entirely clear that what is shown is only part of the token that will be used if you select it.

if it's not a difficult change and you don't want it in the main repo

It would be semi-involved, I believe.

and for this to be disabled by default but enabled via a config flag

We don't really do config flags like this, as a philosophical point.

I'm also not against doing the PR if you can give me suitable pointers to what to do.

I would be against accepting such a PR. I do not believe this should be changed.

That said, I'm not against a hack which allows me to set the colours such the initial text is barely visible and the actual contents are brightly coloured, though this is far from desirable.

Set $fish_pager_color_prefix. E.g. set -g fish_pager_color_prefix 444444 to set it to dark grey.

@AlexSWall
Copy link
Author

I'm not arguing it's more, or even at all, consistent. I'm suggesting, if you ask the majority of people which they find easier to read, I expect the (clear) majority will come down on preferring the latter, as they won't have such philosophical convictions and 100% of the content is relevant, as opposed to less than 50% (with the irrelevant parts of it in bold and the relevant parts greyed out). Literally over 50% of it is the same duplicate text. I'm surprised you don't see any issue at all from the non-philosophical standpoint?
And perhaps you're able to quickly discard all of the unhelpful information, but can you see that this won't hold for everyone, and it'll be exacerbated for some people such as those with dyslexia, those with visual impairment, etc.?

I'm happy to break your philosophical convictions on my own branch if someone can give me the pointers to do so, if it's not too involved. That said, I do not believe I'll be even close to the only one that wants it.

Thanks for the fish_pager_color_prefix command. As this will change it in all cases, it's not the most desirable fix. If I could localize it for just when doing cd tab completion somehow, I'll set it to my background colour and consider it a grim hack that I can live with.

I'd be interested in what more people think on the desirability of this.

@faho
Copy link
Member

faho commented Jan 6, 2022

I'm suggesting, if you ask the majority of people

Voting isn't a good way to make design decisions.

Literally over 50% of it is the same duplicate text

If you want to make a suggestion for a better way to figure out how much of the prefix to show, that's fine. I'm simply opposed to not showing anything at all.

Why? Because for every other completion if it shows a thing that's what will be inserted. So when I see "Videos/", I expect it to insert "Videos/". So if I then select it and get "/home/alfa/Videos", that's surprising. So that now means I need to look up to see what is already in there, but only for the file or cd completions. That's the issue with the inconsistency!

So showing even just …/ in the prefix color would be preferable to showing nothing. It's a hint that "oi, these aren't the full tokens!".

This would presumably be done in pager.cpp, probably in print_max.

with the irrelevant parts of it in bold and the relevant parts greyed out

If you want to propose a better color for one or all of our colorschemes, that's fine, too. That's probably a good idea.

@thithib
Copy link

thithib commented Jan 6, 2022

Hopefully it's not too controversial to say that the first is objectively worse than the latter here

It is, I do not believe it is worse at all.

The first is consistent with every other completion, and it makes it entirely clear that what is shown is only part of the token that will be used if you select it.

But it's useless information that only clutters the pager.

That said, I'm not against a hack which allows me to set the colours such the initial text is barely visible and the actual contents are brightly coloured, though this is far from desirable.

Set $fish_pager_color_prefix. E.g. set -g fish_pager_color_prefix 444444 to set it to dark grey.

As I said in #8617, this also applies to tokens that are not path components. What about allowing a different color for path prefixes, since they're already handled differently (truncated), with a new fish_pager_color_prefix_path variable?

@AlexSWall
Copy link
Author

Voting isn't a good way to make design decisions.

I agree. I'm just saying that the stance of 'it is [too controversial], I do not believe it is worse at all' is not all there is to this as I think a lot of other users will feel very similarly; at the end of the day the shell is for people to use, so it's good to take note of what many users would like.

If you want to make a suggestion for a better way to figure out how much of the prefix to show, that's fine. I'm simply opposed to not showing anything at all.

I'm following (I believe). I think the issue is, shells such as zsh and bash don't separate what's left to complete from what's already written, but just give the entire set of choices to choose from (narrowed down if you've already included some). I'd love this behaviour, just for cd if nothing else. Could this setup be made as a plugin that modifies the cd function? Is there anything in fish that does it this way already? I'm not against a hacky plugin that checks whether the current command is cd (possibly by setting an environment variable elsewhere if needed), and then changes the autocompletion behaviour accordingly.

If you want to propose a better color for one or all of our colorschemes, that's fine, too. That's probably a good idea.

I don't suppose you know the name for setting the colour of the completion choices, as opposed to the above which was for the prefix?

@thithib
Copy link

thithib commented Jan 6, 2022

Literally over 50% of it is the same duplicate text

If you want to make a suggestion for a better way to figure out how much of the prefix to show, that's fine. I'm simply opposed to not showing anything at all.

What about adding a configuration option for the length of the truncated path, that people could then set to 0?

Why? Because for every other completion if it shows a thing that's what will be inserted. So when I see "Videos/", I expect it to insert "Videos/". So if I then select it and get "/home/alfa/Videos", that's surprising. So that now means I need to look up to see what is already in there, but only for the file or cd completions. That's the issue with the inconsistency!

You've already made path components a particular kind of prefix since you truncate them. So one could argue it's already inconsistent. Why did you make that choice initially?

Plus, "what is already in there" is what you typed or selected just before, so why would you need to look it up? You are interested in the next path component that can be selected.

@faho
Copy link
Member

faho commented Jan 6, 2022

I'm following (I believe).

I don't think you follow. What I mean is this: Our pager currently figures out how much of the prefix and the rest of the candidate to show, and ellipsizes the prefix accordingly.

Since you keep saying things like "over 50% of it is the same duplicate text", well, why not fix that and change the pager so it shows less of it? Figure out when and how it should shorten this in a nicer way.

Just be sure to leave a prefix to indicate there is more. This could even be given special knowledge of paths and deprioritize the prefix, as long as it leaves a …/ intact.

Could this setup be made as a plugin that modifies the cd function?

The cd function itself has no say here, it's the core completion logic.

I don't suppose you know the name for setting the colour of the completion choices, as opposed to the above which was for the prefix?

$fish_pager_color_completion, see https://fishshell.com/docs/current/interactive.html#pager-color-variables.

What about adding a configuration option for the length of the truncated path, that people could then set to 0?

Like I said: We don't do those. I realize this is quite a culture shock, coming from zsh which is all-the-options-all-the-time, but we try very very hard not to introduce configuration options, especially not for tertiary things like this.

You've already made path components a particular kind of prefix since you truncate them

We truncate everything after a certain length, not just paths. Also the main issue is showing no indicator that the given candidate isn't the full token.

Plus, "what is already in there" is what you typed or selected just before, so why would you need to look it up?

I need to keep the context that there is more in my head. And if there is no indicator of that (not even a .../), then that's easy to forget and become confused.

@AlexSWall
Copy link
Author

Our pager currently figures out how much of the prefix and the rest of the candidate to show, and ellipsizes the prefix accordingly.

Since you keep saying things like "over 50% of it is the same duplicate text", well, why not fix that and change the pager so it shows less of it? Figure out when and how it should shorten this in a nicer way.

Just be sure to leave a prefix to indicate there is more. This could even be given special knowledge of paths and deprioritize the prefix, as long as it leaves a …/ intact.

I'm happy with leaving the ellipsis prefix, but I figured that you'd still want some of the prefix for the currently completed result? Or is that treated separately and not considered the 'prefix' here? For example, if I just set the colour of the prefix to my background colour and I tab complete with some of the target already typed, I don't get my suggestions including what I've previously typed, e.g.
Screenshot 2022-01-06 at 13 09 02

Would this not actually be an issue with what you're suggesting, as it'll always keep the initial part of the match targets? Or did I indeed understand correctly?

$fish_pager_color_completion, see https://fishshell.com/docs/current/interactive.html#pager-color-variables.

Perfect, thanks.

Like I said: We don't do those. I realize this is quite a culture shock, coming from zsh which is all-the-options-all-the-time, but we try very very hard not to introduce configuration options, especially not for tertiary things like this.

To me, my suggestion doesn't seem much different from fish_prompt_pwd_dir_length?

Also the main issue is showing no indicator that the given candidate isn't the full token.

I'm not against including an indicator. Having a preceding ellipsis doesn't make it more unreadable, which is my main concern.

I need to keep the context that there is more in my head. And if there is no indicator of that (not even a .../), then that's easy to forget and become confused.

Presumably this would be on the command line, though? This is how people get by in all other shells.

@faho
Copy link
Member

faho commented Jan 6, 2022

I'm happy with leaving the ellipsis prefix, but I figured that you'd still want some of the prefix for the currently completed result?

I'm simply saying to find some shortening logic that always leaves some prefix, even if it is just "...". Mostly this involves making shortening more aggressive, especially when it's about paths (where it should probably be ".../"). But for simple cases where it's just "Documents/Backups" and "Documents/Videos" we could probably leave more.

I.e. in your case it should probably leave something like ".../ba".

Maybe the logic could be something like: If the completion either is a path (came from the file completion logic) or looks like one (i.e. has a slash), de-prioritize everything up to the last "/" of the prefix. If that's very long or we are in need of space, make it ".../".

Presumably this would be on the command line, though? This is how people get by in all other shells.

Like I said, looking at the command line requires looking up, out of the pager. Hence leaving the indicator that there is something to look up for.

I am aware that people get by in other shells, but they get by with a lot of other things we do differently as well. How other shells do it isn't all that important, to be honest.

@thithib
Copy link

thithib commented Jan 6, 2022

Our pager currently figures out how much of the prefix and the rest of the candidate to show, and ellipsizes the prefix accordingly.

Since you keep saying things like "over 50% of it is the same duplicate text", well, why not fix that and change the pager so it shows less of it? Figure out when and how it should shorten this in a nicer way.

Just be sure to leave a prefix to indicate there is more. This could even be given special knowledge of paths and deprioritize the prefix, as long as it leaves a …/ intact.

So at least you partially agree that there's too much redundant information currently?

What about adding a configuration option for the length of the truncated path, that people could then set to 0?

Like I said: We don't do those. I realize this is quite a culture shock, coming from zsh which is all-the-options-all-the-time, but we try very very hard not to introduce configuration options, especially not for tertiary things like this.

Yes, I understand and I totally agree. FWIW I want to stop using zsh because I don't like relying on Oh My Zsh and zsh is just too much customizable for me to even start writing my own config. So I'm trying fish and I love it, it's almost perfect for me out of the box and I surely don't want it to gain lots of config options over the years.

It was a bit hard to get used to your history search but now I truly love it. The autocompletion/pager is the last thing that bothers me and stops me from adopting fish: it doesn't expand globs (but I hope it will someday #751), it doesn't colorize files according to their types (I also hope it will someday since you don't seem entirely against it according to #8251 and #2868) which prevents me from quickly spotting a file I'm looking for (e.g. build artifacts, that are executable so colored in green by my zsh, images colored in magenta, directories in blue, fifos in yellow, libs in cyan, etc.) and lastly there's this prefix issue.

You've already made path components a particular kind of prefix since you truncate them

We truncate everything after a certain length, not just paths. Also the main issue is showing no indicator that the given candidate isn't the full token.

Plus, "what is already in there" is what you typed or selected just before, so why would you need to look it up?

I need to keep the context that there is more in my head. And if there is no indicator of that (not even a .../), then that's easy to forget and become confused.

Well I respect that and I guess our brains just work differently then. I don't need to have context in the pager, I know I'm currently completing a path (because if there's a prefix it means I've already typed or selected the beginning of a path myself and I then hit Tab so I know that), and I do have context on the command line right above if I ever need to remember what I was doing a moment ago. What confuses me is actually having the same prefix displayed many times and making less obvious what I have in this directory or what next letter I can type to filter candidates…

One thing to satisfy us both and comply with your philosophy would be an additional color variable like fish_pager_color_prefix_path (I don't think you answered to this suggestion I made in a previous comment, but I'm sorry if I missed your point). It would at least allow me to make prefixes almost invisible, but keep whole commands.

@faho
Copy link
Member

faho commented Jan 6, 2022

One thing to satisfy us both and comply with your philosophy would be an additional color variable like fish_pager_color_prefix_path

Tbh... shortening seems like the better idea here. Having a second prefix color just for paths is awkward.

This has one of the major downsides of options: You need to know about it. That improves things only for the people who went looking for it. It's a cop-out that leaves the responsibility for making things better with the user.

@thithib
Copy link

thithib commented Jan 6, 2022

One thing to satisfy us both and comply with your philosophy would be an additional color variable like fish_pager_color_prefix_path

Tbh... shortening seems like the better idea here. Having a second prefix color just for paths is awkward.

This has one of the major downsides of options: You need to know about it. That improves things only for the people who went looking for it. It's a cop-out that leaves the responsibility for making things better with the user.

It would just be an additional variable in this table… It would be as much discoverable as existing color variables.

Regarding your point of shortening but only when it's too long, it doesn't fix the issue of allowing to rapidly spot e.g. the next character I can enter to filter completion candidates.

Also, how would you feel about another symbol than ellipses? Something that would both make it obvious for you that there is context (and "that there is something to look up for", as you put it) but at the same time not make the next candidates less discernible?
And you could make this symbol customizable (just kidding ;))

@faho
Copy link
Member

faho commented Jan 6, 2022

Also, how would you feel about another symbol than ellipses?

We currently use ellipses here and I don't see a good reason to change it. They are a good symbol for "there's more".

@floam
Copy link
Member

floam commented Jan 17, 2022

I find the prefixes here visually and kinda cognitively noisy, I get where he is coming from. I think the other screenshots are easier to grok what is going on at a glance. If they truncated at exactly a path separator that seems better.

As in

…/foo  …/bar  …/baz

@AlexSWall
Copy link
Author

It seems everyone would be happy with this choice of having just .../ as the prefix to the file/directory/etc. names(?).

If that's indeed the case, I'm wondering how we'd do it. I have no experience with the codebase; what's the best approach for it? I'm not against contributing to doing it if it's feasible for a new contributer, but I'd like to ensure it fits in philosophically with whatever would be needed to get the PR accepted.

@krobelus
Copy link
Member

There's another side of this coin. If there are very few completions, the current ouput looks better IMO, especially for new users. (Also I think …/ looks a bit enigmatic at first)

$ ls ~/Downloads/
…ownloads/archlinux-2022.01.01-x86_64.iso  …ownloads/ubuntu-21.10-desktop-amd64.iso
$ ls ~/Downloads/
…/archlinux-2022.01.01-x86_64.iso  …/ubuntu-21.10-desktop-amd64.iso

Though of course with many completions and for more experienced users the proposed output is better.

Overall I think the change is fine although I don't think it's a strict improvement. When implementing, look for common_prefix in src/reader.cpp.

Observe that PREFIX_MAX_LEN is hardcoded to 9 characters.
How about we scale it down as the number of completions increases?

$ ls ~/Downloads/
…ownloads/1 …ownloads/2
$ ls ~/Downloads/
…wnloads/1 …wnloads/2 …wnloads/3 …wnloads/4
$ ls ~/Downloads/
…oads/1 …oads/2 …oads/3 …oads/4
…oads/5 …oads/6 …oads/7 …oads/8
$ ls ~/Downloads/
…/1 …/2 …/3 …/4
…/5 …/6 …/7 …/8
…/9 …/10 …/11 …/12
…/13 …/14 …/15 …/16

OK that doesn't look so good (it's a rough sketch) but I think it's worth exploring. Maybe prefix-len = max(1, 10 - n).


Perhaps we should even remove the entire shared prefix, disregarding the /? (Or only if there are many completions).

$ ls ~/Downloads/
…archlinux-2022.01.01-x86_64.iso  …ubuntu-21.10-desktop-amd64.iso
$ git switch some-really-really-really-long-
…branch1 …branch2

@AlexSWall
Copy link
Author

AlexSWall commented Jan 26, 2022

I know I like the prefix just being …/ consistently (which is also probably a little simpler to implement) because the path completion options one has and a prefixed …/ to explain it's part of the path is precisely the information needed for a user—I find the context on the command line that one's just typed/chosen alongside them having manually pressed tab for the completion options to be ample context. Anything else is duplicated for each option in addition to already being on the command line, which is excessive to me.

And if some people (but not all) really don't agree with that, are we certain we don't want this to be configurable? Feels like it may be hard to reach a consensus on something so subjective, which will mean this ticket just stalls indefinitely and never gets fixed.

@faho
Copy link
Member

faho commented Jan 26, 2022

And if some people (but not all) really don't agree with that, are we certain we don't want this to be configurable?

Yes. This is not an option that people would really find.

This is one of those side-options for a rather specific feature that most people don't care about but it clutters the list of options and is hard to know about for the people who would benefit from it.

So we should not implement it. Pick the best setting and make that the only choice instead. And if that's the current way of doing things then so be it.

Making it an option makes the person asking for the option feel good because they get to pick their favorite choice, but that's simply not the way we do things.

@faho
Copy link
Member

faho commented Jan 26, 2022

OK that doesn't look so good (it's a rough sketch) but I think it's worth exploring. Maybe prefix-len = max(1, 10 - n).

Really, what we want is to avoid showing too much prefix for too little "suffix". Don't show Downloads/1 Downloads/2 Downloads/3 because that makes the actual important bits -1 2 3 hard to see.

So that means either specifically handle paths or path-looking things because those have components that you already know, and we can quickly skip to the last "/", or to look at the length of the prefix in relation to the "suffix" (i.e. the actual completion candidate). (I'm assuming the latter is untenable because it requires scanning through all the candidates before deciding on a prefix length)

The issue with making it scale with the number of candidates is that it doesn't look at the relative length - that example with Downloads three times is only three candidates, but it's already a bunch of redundancy.

@floam
Copy link
Member

floam commented Jan 26, 2022

I'm also not a fan of making such a detail configurable. To me, that is like giving up.

@krobelus
Copy link
Member

Really, what we want is to avoid showing too much prefix for too little "suffix".

Good point, I agree that it's actually about the relative length

or to look at the length of the prefix in relation to the "suffix" (i.e. the actual completion candidate). (I'm assuming the latter is untenable because it requires scanning through all the candidates before deciding on a prefix length)

we we already look through all candidates and only truncate the prefix at the end. So that would work well.
Maybe try prefix_max_len = min(len(suffix) for suffix in completions) * 2/3.

If that doesn't work, I think we agree that …/ is fine (perhaps also for other separators like …=, see resolve_auto_space()).

@rudenkornk

This comment was marked as duplicate.

@AndrejSoroj

This comment was marked as resolved.

@xiebruce

This comment was marked as duplicate.

@huyiqun

This comment was marked as duplicate.

@esn89
Copy link

esn89 commented Jun 12, 2024

Hi, is there a solution for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants