[Search directory] fix quick directory change for hidden directories

[Laptop765]

Fixes #171 by making . optional but / required when deciding not to prepend ./.

[Laptop765]

I'd have added tests but the machine I'm on is currently locked at fish 3.1.2 so I can't actually run/test against HEAD. I have a fork of v6.5 that I made these same changes on and seems to be working as expected.

[PatrickF1]

Thanks for the PR!

[Laptop765]

Happy to help!

[kidonng]

I think some later changes (especially #133) makes #119 unnecessary. In other words, we no longer need to check the selected paths again because they never start with literal tilde (~) and such.

What I'm saying is that we should just remove the check entirely.

[PatrickF1]

Sorry for the delay. This is on my radar but I've been busy or sick. I'll get this merged eventually

[Laptop765]

No worries, take care of yourself! I have a fork (and am stuck on an older fish at work so I'm on v6.5 anyway) so I'm not blocked; I just wanted to contribute back.

[PatrickF1]

Something is off. The plugin doesn't prepend ./ anymore when there is a commandline token. E.g. if you start typing in Doc and you select Documents, you'd expect ./Documents but it outputs Documents.

[Laptop765]

Ahh good catch!

It looks like when nothing is typed, both $commandline_tokens and $current_token are blank whereas if something is typed before activating the search $commandline_tokens contains what was typed but $current_token is still blank. I'm not sure where $current_token is coming from.

I wonder if it might be better to do something like test -d $file_paths_selected to check if the selection is a directory rather than the current approach?

[Laptop765]

I think it might be missing set current_token (commandline --current-token) after the set commandline_tokens? I'm pretty new to fish so I'm not fully sure what this code is supposed to be doing or if that's a correct fix.

[Laptop765]

If there are two tokens, what is the expectation of completing a directory? Activation happens where the | is in the below examples representing the cursor position

• First token: Doc| test
• Subsequent token: test Doc|

Is the expectation that it should complete as ./Documents in both cases or no?

• If yes we can remove the test "$commandline_tokens" = "$current_token" check and always replace the current token with ./Documents.
• If no, we can test (count $commandline_tokens) = 1 instead and remove $current_token so that we only replace with ./Documents if it's the only token.

[kidonng]

There used to be a set current_token (commandline --current-token), and was removed in 160e862 (#119), for no good reason.

[PatrickF1]

I wonder if it might be better to do something like test -d $file_paths_selected to check if the selection is a directory rather than the current approach?

Yeah, this was the old approach. I forgot why we changed it. I'd be open to going back and just append a / to all selected directories. It be a little un-performant if selecting tons of results, but that should be rare @kidonng any thoughts?

Is the expectation that it should complete as ./Documents in both cases or no?

I only wanted to prepend ./ to the directory for the purposes of when the user was looking for exactly one folder to cd to (if you hit enter when the only token is ./directory, it changes to directory). So no. And we can't just do test (count $commandline_tokens) = 1 because if the user has something |, and they select Documents, the command line becomes something ./Documents they wouldn't be able to hit enter to cd into Documents was selected.

There used to be a set current_token (commandline --current-token), and was removed in 160e862 (#119), for no good reason.

Good catch! Thanks!

[PatrickF1]

In other words, we no longer need to check the selected paths again because they never start with literal tilde (~) and such.

Actually, ~ can expand to /Users/me, in which case we still need the check.

[kidonng]

I posted that comment because I did some tests by hand. But perhaps I should give it another shot and post the results here.

[kidonng]

My previous comment is incorrect because the ./ prepending never happened due to the empty $current_toke variable.

@Laptop765 sorry for the misguidance. Please revert my suggestions. After that the PR looks good.

[kidonng]

I wonder if it might be better to do something like test -d $file_paths_selected to check if the selection is a directory rather than the current approach?

Yeah, this was the old approach. I forgot why we changed it. I'd be open to going back and just append a / to all selected directories. It be a little un-performant if selecting tons of results, but that should be rare @kidonng any thoughts?

"The old approach" never entered the codebase and stayed at the original PR. No need to revert since we have no issues with current approach (code is a little broken though).

[PatrickF1]

Ok sorry for the delay. I figured out a good way to fix this. Unfortunately, it's gonna be a decent amount of work so probably better if I start a new PR. Thanks for your hard work @Laptop765 and @kidonng