-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add file browser #11285
base: master
Are you sure you want to change the base?
Add file browser #11285
Conversation
Would it be possible to implement a file browser with this methodology instead? https://github.com/stevearc/oil.nvim It uses a buffer/pop to navigate and edit files like you're inside a buffer. |
@drybalka this is awesome.. thanks for the effort. Can you please post a screenshot as to how it looks ? Basically I am interested to show how nested directories/files are being presented. |
I am not a maintainer of helix, but in my opinion this is rather a plugin functionality. First of all, it would be hard-ish to implement and therefore to maintain. Secondly, buffers are primarily used for text editing and one does not usually need to create/delete/rename files so much. |
Well, the idea was to make it look and behave like the original telescope-file-browser, so you may look at the showcase video there (just without all pretty-niceness as this is just a proof-of-concept). As for a real screenshot it looks (quite bare bones) like this: In other words, both the picker and the preview (if a dir is selected) show the contents at depth 1, similar to how |
I kind of like just how simple this change is! It reuses existing UI components and allows exploring the file tree without adding any of the heavier features. |
helix-term/src/ui/mod.rs
Outdated
if let Ok(files) = directory_content { | ||
for file in files { | ||
if injector.push(file).is_err() { | ||
break; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This style is used by the file picker because finding files is a potentially long-running iterator and we might use the injector to push some files asynchronously after a timeout. Since you've already collected the directory contents above you should pass that vec as the third argument to Picker::new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I believe this function should return a result and pass up the error from directory_content
. Currently if you fail to list the directory contents a blank picker opens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestions, simplified the code, thank you!
I think it would be useful to have the equivalent version of It could be something like this: fn file_browser_in_current_buffer_directory(cx: &mut Context) {
let doc_dir = doc!(cx.editor)
.path()
.and_then(|path| path.parent().map(|path| path.to_path_buf()));
let path = match doc_dir {
Some(path) => path,
None => {
cx.editor.set_error("current buffer has no path or parent");
return;
}
};
if let Ok(picker) = ui::file_browser(path) {
cx.push_layer(Box::new(overlaid(picker)));
}
} |
I think this can even be the default behavior, thanks for suggesting! Not sure when I would even need to open a file browser in the project root. I deliberately wanted to keep this PR simple and feature-poor, but your suggestion is quite simple and I think it is worth it. |
@drybalka This is perfect with the change to the current buffer directory. It's similar to good ol' netrw for quick navigation and covers 95% of the needs for me. The more advanced stuff (copy, paste, create, delete, etc) can be covered by a mix of #11164, shell scripts, wezterm, and the yazi file explorer. Thanks for your work! |
@drybalka found a small issue when testing: if the cursor is on a binary file, the preview will mess up the view and leave random characters everywhere. Maybe binary files can be excluded from preview somehow? |
@zegervdv I am using the same file previewer as file_picker, so the problem is probably in the previewer itself. Although as far as I tested the usual .jpg, .pdf, and executables are all correctly previewed as |
It would be nice to add support for going straight to the root of the project if the current buffer hasn't been saved yet. I went to test the file browser functionality by escaping the file picker when calling Screen.Recording.2024-09-10.at.08.18.51.mov |
Actually, I can't get this to work at all? Running on MacOS. Screen.Recording.2024-09-10.at.08.47.35.movHere is the log from |
Well, this is actually the behavior I get when using the command palette for the file_picker as well, I guess the palette is somehow buggy in this regard. I just mapped it to some keymap and tested like that. But anyway, even if you map it correctly and then try to open the file_browser after |
Actually in current latest version of helix, no file picker can open from the command pallete (tried in nixos and archlinux). It seems like another bug. |
I have the same reported issue when trying to open the browser from the command palette. Works great when opened from a keymap though. I do have a couple of suggestions to consider
|
So stoked to see this @drybalka! 🫶 Probably not worth it at this stage as merging the base feature is higher priority, but it'd be cool be be able to classify the entries with icons. Having a trailing slash on dirs makes sense, but hugely useful IMO to when something is linked, and being able to visually filter on filetypes. Kinda like I could probably take a look at doing this when this is in if you aren't feeling it :) |
5695166
to
b743ff3
Compare
@summersz Adding slash to dirs is a great idea, thank you! Using other keys in a picker is harder. The current design does not allow picker-specific keymaps, and even remapping the existing keys is not allowed yet. I will leave this idea for the future, as pickers refactor should come at some point. @m0ar I also really like the idea with icons! The file_browser picker shares a lot of code with the file_picker, so if the icons would work there then most probably they will also work here. I don't want to complicate this PR with icons at this point, but you may already try implementing them for the file_picker. |
bb00c5a
to
68b252a
Compare
To use this, you can add the following keybinding to your config ( [keys.normal.space.space]
f = "file_browser" Now, you can double-tap Note that you cannot open the file browser from the command palette due to #4508 (as discussed above). |
My preference is for @NikitaRevenco's suggestion. I have been using this for a while and exclusively with file_browser_in_current_buffer_directory. This implementation is similar to netrw which opens at the buffer directory, which I think this is most suited for. This is still inline with f/F, where the capitalization opens a narrower scope. I think the browser is more useful at working and buffer directories so makes it sense to deviate. |
@m0ar Thank you for the cupcake! I deliberately did not add the default key mappings because they are often a matter of taste. I would prefer to get a green flag from the core maintainers with a wider vision for helix first, or maybe one of them may even add a commit with the mappings themself before the merge, as it may be faster than writing a github comment =) I personally use |
127a588
to
52a9cef
Compare
@the-mikedavis Thank you very much for the review, I really appreciate the insights! I added the styling for dirs, as you suggested, and also removed canonicalization and unnecessary is_dir syscalls. Now the type signatures are a bit polluted with I also had to rebase to the newest master to get the styling code, but I did not modify any commits, only added the last one. Have a Merry Christmas! |
thank you for rebasing @drybalka! for us forkers that is very valuable :3 |
`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests. Check it out here: https://github.com/NikitaRevenco/patchy
`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests. Check it out here: https://github.com/NikitaRevenco/patchy
`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests. Check it out here: https://github.com/NikitaRevenco/patchy
`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests. Check it out here: https://github.com/NikitaRevenco/patchy
); | ||
return; | ||
} | ||
cx.editor.set_error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't call this an error, honestly. For example I'm frequently doing hx .
and then opening the file browser, since i am in a [scratch]
, everytime I do that it adds a red message to the statusline, so it looks like something went wrong, but really it's just fine
I think that this call to set_error
can just be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be fair, this function is called file_browser_in_current_buffer_directory
, so this error is quite justified to inform the user that the [scratch]
buffer does not have a dedicated directory. In my opinion, it would be confusing to remove this message for cases when something actually goes wrong.
What you want to call after hx .
is probably file_browser_in_current_directory
or simply file_browser
. Now I agree, that it is more convenient to have a single key combination that covers all file browsing needs in one go, and arguably file_browser_in_current_buffer_directory
already does that for the price of showing you a somewhat-irritating-but-easily-ignorable error message. In an ideal world one would probably have to wait for when programmable config will be available in helix and code the desired behavior themself.
`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests. Check it out here: https://github.com/NikitaRevenco/patchy
`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests. Check it out here: https://github.com/NikitaRevenco/patchy
`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests. Check it out here: https://github.com/NikitaRevenco/patchy
`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests. Check it out here: https://github.com/NikitaRevenco/patchy
@@ -393,6 +393,9 @@ impl MappableCommand { | |||
file_picker, "Open file picker", | |||
file_picker_in_current_buffer_directory, "Open file picker at current buffer's directory", | |||
file_picker_in_current_directory, "Open file picker at current working directory", | |||
file_browser, "Open file browser in workspace root", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to set a default keybind of <space>.
for this. Thoughts @archseer? @pascalkuthe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me but I haven't looked at this PR much yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think <space>e
could also work. Could have a <space>E
, similar in nature to the file picker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created a PR that adds mappings and fixes the docs check drybalka#1
This is a minimal implementation of the file browser, which would probably cover a lot of requirements in #200. The whole thing works analogous to the https://github.com/nvim-telescope/telescope-file-browser.nvim as suggested in this comment. Even though the resolution of the discussion seems to be "file tree/browser is too hard, it should be implemented as a plugin", I feel like my changes are quite small and natural to be considered for adding to the core nonetheless.
The implementation simply builds on the existing file picker and only modifies 3 files, so the added maintenance burden should be quite small. The code itself is not particularly elegant (in my rather inexperienced opinion), but I did not want to over-complicate things. This is also the reason why some features might be missing.
Feel free to modify this PR or simply make suggestions, I'd be happy to improve it. This is also my first PR here, so sorry if I miss anything.