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

Limit file search to directory under cursor if it has a trailing slash #75

Merged
merged 14 commits into from
Jan 9, 2021
Merged

Conversation

kidonng
Copy link
Contributor

@kidonng kidonng commented Jan 6, 2021

Derived from #70 (comment)

@imr0
Copy link

imr0 commented Jan 6, 2021

Thanks for the feedback in #70! My preferences for why using completion this way have basically been summarized by @paddor in #70 (comment), but I will also reiterate them here:

  • A lot faster!
  • Works with queries containing parent–sibling relationships between directories (i.e. ../ or ./ in the queries).
  • More display space for files under the given directory, which looks much cleaner, especially if the preview window is open.
  • Can exploit muscle memory for TAB completion and then invoking fzf.
  • For—in my opinion—edge cases when the user is searching for a query elsewhere with the exact name of a directory that already exists in the current working directory, hitting Ctrl+f first and then typing in the query is still an option.

@PatrickF1
Copy link
Owner

PatrickF1 commented Jan 7, 2021

Sorry for the slow turnaround time, been a heck of a day at work...and will continue to be for a while so please be patient.

Anyway, for now, I'm not sold on this. Why? It's not that I don't like the idea. But there are serious downsides when considering the plugin holistically

  • The change adds a lot of complexity to the find files function, which I have to maintain. And I don't want to invest a ton in this feature anymore because I am realizing more and more this problem space of navigating directories is best solved by a CLI file manager (e.g. nnn, ranger, midnight commander, lf, Nerdtree, and many many more). It's a very saturated market. Navigating directories has bene solved a million times over, some solutions are pretty awesome (I love how nnn does it), and I really don't want to re-invent the wheel. I myself am going to move to nnn and probably stop using this feature myself probably. And I think other people should too. There's no way I can or want to build/maintain anything close to the feature set that nnn provides.
  • This is more like adding a quirk to the search files functionality. It's not easily discoverable unless someone looks at the code or follows the releases notes or scans through of the readme. And does anyone really do those things for such a small plugin? I doubt it. I don't want to surprise users with the many quirks of search files, because now there are really 3 modes: search for files with no current token, search for files that replaces the current token, search for files using the current token as a directory.
  • Users will probably a hard time remembering and mastering all the quirks of this function. And my goal has always been to KISS so that using the plugin is a no-brainer.
  • This needs to be documented and clearly and quickly taught to all users.

Sorry for i'm being harsh. I hate that you already put in the work. But I won't merge it as is. However, I am open to a compromise. Maybe if we use a key bind to reload fzf's input list with the contents of the directory in the query? e.g. fzf --bind ctrl-f:reload(fd $current_token)?

CC: @paddor, @imr0

@kidonng
Copy link
Contributor Author

kidonng commented Jan 7, 2021

  • For—in my opinion—edge cases when the user is searching for a query elsewhere with the exact name of a directory that already exists in the current working directory, hitting Ctrl+f first and then typing in the query is still an option.

We can add a little check here. If there is no other match in the currect directory, we will use the entered directory as a base.

@kidonng
Copy link
Contributor Author

kidonng commented Jan 7, 2021

  • The change adds a lot of complexity to the find files function, which I have to maintain.

I don't think this is that complex, but yeah, I'm not the one to say it.

navigating directories is best solved by a CLI file manager

We are not building a file manager. This is a file picker after all, any only recently it got addition so it can be used for quickly navigating to folders.

Third-party program can't intergrate with Fish at this level (unless you would like to keep the program open), which means this function will never become useless.

3 modes: search for files with no current token,

That is not a mode, we only save the user from typing ./ themselves. And this PR is not adding a new mode either, see explanation below.

I agree with the rest of narration. This PR is not meant to be accepted as-is, I'm just throwing in my thoughts.

@kidonng
Copy link
Contributor Author

kidonng commented Jan 7, 2021

I just made a change which gives a compelling reason why this will be a nice addition with no cost.

The base directory will only be changed to the token under cursor if:

  • The token under cursor is a directory
  • The token ends with slash (/)

Why I call this a compelling reason? Because when you open the search with src/, you are definitely searching inside src, not the currect directory, which makes changing the base directory a natural choice.

And I want to provide a real life usage for @imr0's forth point:

  • Can exploit muscle memory for TAB completion and then invoking fzf.

As you may know macOS has a large ~/Library which pretty much stores everything generated by programs. This makes searching in currect directory very inconvinient if you are at home (~). With this PR I can type, for example, .con, and hit Tab to let Fish turn it into .config/, then I can search in .config only, which is much faster.

@PatrickF1
Copy link
Owner

Just tested. The file previews broke :(
Otherwise I like the idea and will definitely merge once it's passed testing.

@kidonng
Copy link
Contributor Author

kidonng commented Jan 8, 2021

@PatrickF1 I have fixed the preview (which is easier than I expect).

@kidonng
Copy link
Contributor Author

kidonng commented Jan 8, 2021

Yeah I kinda messed up my local Git history when I merged master and did my first force push. Never mind, I fixed it.

@kidonng kidonng marked this pull request as draft January 8, 2021 09:03
@kidonng kidonng marked this pull request as ready for review January 8, 2021 09:19
Copy link
Owner

@PatrickF1 PatrickF1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

Copy link
Owner

@PatrickF1 PatrickF1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it still doesn't work. I would have thought the fix would work but now I don't know what's wrong.
Screen Shot 2021-01-08 at 2 21 40 PM

@PatrickF1 PatrickF1 mentioned this pull request Jan 9, 2021
@kidonng
Copy link
Contributor Author

kidonng commented Jan 9, 2021

Works on my machine™️

image

...and in CodeSpaces (which is standard Fish 3.1.2):

image

Copy link
Owner

@PatrickF1 PatrickF1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you're right--it works again now--not sure what I did wrong.

@PatrickF1 PatrickF1 changed the title Use the directory under cursor as base directory Limit file search to directory under cursor if it ends in a slash Jan 9, 2021
@PatrickF1 PatrickF1 changed the title Limit file search to directory under cursor if it ends in a slash Limit file search to directory under cursor if has a trailing slash Jan 9, 2021
@PatrickF1 PatrickF1 changed the title Limit file search to directory under cursor if has a trailing slash Limit file search to directory under cursor if it has a trailing slash Jan 9, 2021
@PatrickF1 PatrickF1 merged commit af7b913 into PatrickF1:main Jan 9, 2021
@PatrickF1 PatrickF1 deleted the base branch January 9, 2021 07:21
@kidonng
Copy link
Contributor Author

kidonng commented Mar 14, 2021

Just noticed fzf does this in their own Fish plugin: https://github.com/junegunn/fzf#fish-shell

@PatrickF1
Copy link
Owner

Nice, I'm glad we added it though it's causing quite a bit of maintenance and documentation pain.

hrshtst pushed a commit to hrshtst/fzf.fish that referenced this pull request Apr 22, 2021
…atrickF1#75)

This new behavior allows the user to control the directory in which the search file feature initiates its search. If the current token is a directory and it has a trailing slash (e.g. functions/<CURSOR>), then we assume the user wants to search for files in that directory.
This can be useful when in a directory containing large subdirectories (e.g. on macOS, the home directory contains ~/Library which is very large). Moreover, it also allows searching a parent or sibling directory by using ../ (e.g. ../Downloads/<CURSOR>. 
What's great is that this new behavior is that it complements tab completion. When a user wants to access a file in a subdirectory, they may start typing the name of the directory and hit TAB. When tab completion writes out the directory's path, it will already append a trailing slash for the user, allowing the user to quickly execute the search files feature and search within that directory.
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.

3 participants