helix icon indicating copy to clipboard operation
helix copied to clipboard

File explorer and tree helper (v3)

Open wongjiahau opened this issue 2 years ago • 119 comments

This is the successor of https://github.com/helix-editor/helix/pull/2377 and https://github.com/helix-editor/helix/pull/5566, which implemented focus current file (as per https://github.com/helix-editor/helix/pull/2377#issuecomment-1274439600).

https://user-images.githubusercontent.com/23183656/216097714-e88dc555-84ec-4ab2-ac92-21ca6fc6f788.mov

wongjiahau avatar Feb 01 '23 16:02 wongjiahau

This is great! I have some feedback on it, just as a user, not maintainer of the project. :)

  • explorer feels like a strange name for it. IMO a better name would be file-tree since that's what it is. We already have a file-picker after all :^)

  • I like that it can be floating or docked to the left!

    • Having an option to dock on the right, instead of just the left, would be excellent
    • My gut says that this may be easier for people who's culture uses RTL as the normal script direction, and there are also arguments to be made for ergonomics too because it doesn't move your text around when toggling.
  • Calling the positions "embed" and "overlay" feels weird to me. Definitely not what I'd guess the setting values would be. I'd go for "left", "right" and "overlay" (or even "popup") to make it more explicit.

  • It may be easier to read if, and look nicer, if the tree mode had a space between the line and file name. This would give the file name a bit of breathing room.

  • The last item in the tree should use instead of to indicate that the following item is not part of the same subtree. Screenshot 2023-02-02 at 3 06 18 pm

  • Because the CWD path is displayed, the view can scroll left and right when using h/l on file names, which feels strange

The rest of my thoughts are on the UX. I tried to write it up but couldn't quite word it right. Will think about it some more.

clo4 avatar Feb 02 '23 05:02 clo4

Working really well already, great work!

What would be the best way to handle opening files when working in multiple splits?

In the file tree I used in neovim when you had more than one split open, and pressed open file in the tree it would overlay an A and B over the splits and then it would open where you selected.

cd-a avatar Feb 02 '23 19:02 cd-a

When I press space E to open explorer and focus the current file, the file that's highlighted sits at the top of the explorer.

I would suggest that it the active file is centered in the middle, so you can see the context above. Context above is arguably more important due to the containing directory structure.

In the video in the top post it looks like it's working, but on latest commit not for me.

CleanShot 2023-02-02 at 20 56 51@2x

cd-a avatar Feb 02 '23 19:02 cd-a

This is very cool! Thank you for opening a new PR with these changes.

Keybindings

Expand / collapse folders

(currently Enter)

I would find it more natural to

  • open / expand folders in the tree with l / Right and
  • close them with h / Left

(= open / close "doors" for directory vertical movement), since j and k are used for up and down (directory lateral movement).

Scrolling

(currently h / l)

For pure non-movement scrolling in the tree, which is probably rare, combinations with z (known for View mode) should be found. Also holding Shift with h/j/k/l and holding Shift with Left/Up/Down/Right could be used for scrolling.

However, z for view mode is helix lore, while holding Shift is not.

Folding current level

(currently z)

For folding the current level: Alt h / Alt Left, as z is needed for non-movement scrolling and known for View mode.

LeoniePhiline avatar Feb 03 '23 17:02 LeoniePhiline

Perhaps it would be best to make an RFC-style doc with a proposed interaction model, so we can all have a discussion about the UX outside of a code-review context? Once there's something fully spec'd out, I'm sure it'll be much easier to implement without as much code churn -- sounds a lot easier than dealing with all the feedback in a PR 😅

(edit: let me know if this exists already!)

clo4 avatar Feb 04 '23 13:02 clo4

Perhaps it would be best to make an RFC-style doc with a proposed interaction model, so we can all have a discussion about the UX outside of a code-review context? Once there's something fully spec'd out, I'm sure it'll be much easier to implement without as much code churn -- sounds a lot easier than dealing with all the feedback in a PR 😅

(edit: let me know if this exists already!)

Or, alternatively, have something that works merged (an MVP) and iterate over the result in future PRs instead of implementing (or designing) the perfect solution upfront?

marcesquerra avatar Feb 04 '23 15:02 marcesquerra

Calling the positions "embed" and "overlay" feels weird to me. Definitely not what I'd guess the setting values would be. I'd go for "left", "right" and "overlay" (or even "popup") to make it more explicit.

Implemented explorer.position right, and embed is renamed as left, via https://github.com/helix-editor/helix/pull/5768/commits/c446c396453f3fd8f52aab900202b9835806e22e:

https://user-images.githubusercontent.com/23183656/216994807-1bdd3828-4193-4d89-af91-26e3fc87e01a.mov

wongjiahau avatar Feb 06 '23 14:02 wongjiahau

@clo4

explorer feels like a strange name for it. IMO a better name would be file-tree since that's what it is. We already have a file-picker after all :^)

I thought it was strange at first until I realized @cossonleo actually built another view for the explorer besides the default tree-view, which is "list", which behaves like Vim's built-in file explorer, :Explore.

It can be configured via :set explorer.style list.

wongjiahau avatar Feb 06 '23 14:02 wongjiahau

Replying to https://github.com/helix-editor/helix/pull/5768#issuecomment-1416176022:

Yes, I will reconfigure the keymaps by borrowing inspiration from nvim-tree.lua and/or coc-explorer, which I find more natural than the existing keymaps.

wongjiahau avatar Feb 06 '23 14:02 wongjiahau

Thanks @GervinFung for testing out the features.

Changes since last comment

  • Refactor internal data structure to enhance code testability and readability

    • Previous data structure was a Vector-Tree hybrid, now it is purely Tree, with Tree-like API
    • This also allows the following features to be implemented without sorcery
  • h moves to parent instead of scrolling left

  • l steps into folder

  • Reveal current file keymap changed from space E to space e (so that if you want to say delete the current file, you just need to press <space>ed, instead of <space>Ed which is harder to get right.)

  • Remap add file to a

  • Remap add folder to A

  • Remap delete file/folder to d

  • Implement rename (r), can be used to move files too

  • Implement non-destructive close (q), which remembers previous Tree state

  • Implement refresh (R), useful when files are modified externally

  • Remap change root to current child to ] (Inspired by Vim's Ctrl-])

  • Remap change root to parent to [ (inverse of ])

  • Implement go back to previous root (Ctrl-o)

  • Remove List view (can be emulated using [/])

  • Implement resize (+/-)

  • Implement help (?)

  • Enhance filter (f) algorithm

  • Ancestors of current file/folder will be highlighted, which allows users to identify the current folder quickly

  • Resolved conflicts from master branch

What's next?

  • Add integration test
  • Code review
  • Gather feedback (feel free to comment!)

wongjiahau avatar Feb 15 '23 10:02 wongjiahau

Looking great!

I have so many tiny requests related to this, but rather will wait :)

image

Proful avatar Feb 15 '23 10:02 Proful

Awesome work! This already feels a lot better to use.

Here's some feedback on the UX:

  • C-n (down) and C-p (up) should be default bindings (some keyboard layouts make j/k inconvenient)
  • Panics if terminal isn't tall enough for the preview window
  • Creating or renaming a file/folder should let me create intermediate directories, i.e. mkdir -p (vscode supports this, it's a really nice QOL thing!)
  • Both + and = should make the pane wider, to avoid having to hold shift for only one of the resize actions. _ could be bound to shrink it too, for symmetry.
  • Not sure about this one, should space also step into the selected folder? IIRC Onivim 2 had this

As for the visuals, it looks great. You've done a fantastic job. Here's my only UI feedback:

  • I'd love to see if indenting by 2 columns per level would look even better. The single level of indentation may also not be enough for everyone from an a11y point of view, some people struggle to discern differences of only 1 col.

Will let you know if I think of anything else! Thank you for all the work you're putting into this.

clo4 avatar Feb 15 '23 11:02 clo4

It seems really great!

I did encounter some minor issues:

  1. When searching with /, and n/N, it seems the search doesn't wrap around, and simply stops when reaching the end, unlike in the editor
  2. Searching with / only searches visible files, it will only find a file if the directory containing it is expanded.
  3. When filtering with f, it will search recursively, but without expanding directories to show the files that are found.
  4. Filtering with f doesn't hide directories without any matching files.

On top of these minor UX issues, I found that I can crash the editor by highlighting a file below all directories like so:

image

Then I press f and type the name of a file that doesn't exist below my cursor, for example main. Here's a backtrace from the crash:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', helix-term/src/ui/tree.rs:616:30
stack backtrace:
   0:     0x55d78ca0b86a - std::backtrace_rs::backtrace::libunwind::trace::h8f4e76b8218b8540
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/../../backtrace/src/backtrace/libunwind.rs:93:5
   1:     0x55d78ca0b86a - std::backtrace_rs::backtrace::trace_unsynchronized::hcdb6a06d2fd94ab3
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
   2:     0x55d78ca0b86a - std::sys_common::backtrace::_print_fmt::h6986e85b14c0a3c2
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/sys_common/backtrace.rs:65:5
   3:     0x55d78ca0b86a - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hf09049473c7eb0d5
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/sys_common/backtrace.rs:44:22
   4:     0x55d78ca3429e - core::fmt::write::h7b08166a7a14a28e
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/fmt/mod.rs:1208:17
   5:     0x55d78ca050c5 - std::io::Write::write_fmt::hdb3e773b5b1c54df
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/io/mod.rs:1682:15
   6:     0x55d78ca0b635 - std::sys_common::backtrace::_print::h1ceaebbb26a3fab3
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/sys_common/backtrace.rs:47:5
   7:     0x55d78ca0b635 - std::sys_common::backtrace::print::hb5aa1269b1b9bc74
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/sys_common/backtrace.rs:34:9
   8:     0x55d78ca0d30f - std::panicking::default_hook::{{closure}}::hbaa8ce6d79af00fb
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:267:22
   9:     0x55d78ca0d04b - std::panicking::default_hook::hcd65d80b40db7fc0
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:286:9
  10:     0x55d78b079590 - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::hdb1fc527bde2863f
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/alloc/src/boxed.rs:2032:9
  11:     0x55d78b05ba57 - helix_term::application::Application::run::{{closure}}::{{closure}}::h9dfac26d4c4340c0
                               at /home/yonatan/Code/helix/helix-term/src/application.rs:1075:13
  12:     0x55d78ca0da3d - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::ha4979ddfbbe58f68
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/alloc/src/boxed.rs:2032:9
  13:     0x55d78ca0da3d - std::panicking::rust_panic_with_hook::hbb6ac4823ece4d03
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:692:13
  14:     0x55d78ca0d772 - std::panicking::begin_panic_handler::{{closure}}::h325f7839ba4584c6
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:577:13
  15:     0x55d78ca0bd1c - std::sys_common::backtrace::__rust_end_short_backtrace::hacaef3341973bb56
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/sys_common/backtrace.rs:137:18
  16:     0x55d78ca0d4c2 - rust_begin_unwind
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:575:5
  17:     0x55d78b03a6d3 - core::panicking::panic_fmt::hef6845e1540bf16b
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:64:14
  18:     0x55d78b03a7ad - core::panicking::panic::h942b2226feba91aa
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:111:5
  19:     0x55d78b100280 - core::option::Option<T>::unwrap::h6d65497d8f3025af
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/option.rs:778:21
  20:     0x55d78b41e2bb - helix_term::ui::tree::TreeView<T>::get::h09bf2bd05aed1fca
                               at /home/yonatan/Code/helix/helix-term/src/ui/tree.rs:616:9
  21:     0x55d78b41e2ff - helix_term::ui::tree::TreeView<T>::current::heec9c2b2e644ea27
                               at /home/yonatan/Code/helix/helix-term/src/ui/tree.rs:624:9
  22:     0x55d78b613c8f - helix_term::ui::explore::Explorer::render_preview::h114103f315212185
                               at /home/yonatan/Code/helix/helix-term/src/ui/explore.rs:270:20
  23:     0x55d78b617299 - helix_term::ui::explore::Explorer::render_embed::h9222ce9fc01b087c
                               at /home/yonatan/Code/helix/helix-term/src/ui/explore.rs:603:13
  24:     0x55d78b44e6e3 - <helix_term::ui::editor::EditorView as helix_term::compositor::Component>::render::h754a7b81f236f7d5
                               at /home/yonatan/Code/helix/helix-term/src/ui/editor.rs:1473:21
  25:     0x55d78b12166f - helix_term::compositor::Compositor::render::h5442162611243686
                               at /home/yonatan/Code/helix/helix-term/src/compositor.rs:170:13
  26:     0x55d78b05cd04 - helix_term::application::Application::render::{{closure}}::hc5961c4d157ce35b
                               at /home/yonatan/Code/helix/helix-term/src/application.rs:307:9
  27:     0x55d78b055442 - helix_term::application::Application::handle_terminal_events::{{closure}}::hd63a1da3e5ecfc91
                               at /home/yonatan/Code/helix/helix-term/src/application.rs:641:26
  28:     0x55d78b0540d0 - helix_term::application::Application::event_loop_until_idle::{{closure}}::h4a0af7fca3d589f3
                               at /home/yonatan/Code/helix/helix-term/src/application.rs:345:55
  29:     0x55d78b0510ac - helix_term::application::Application::event_loop::{{closure}}::hf42a69835fdab62c
                               at /home/yonatan/Code/helix/helix-term/src/application.rs:324:57
  30:     0x55d78b05b49c - helix_term::application::Application::run::{{closure}}::hf60db97ca90db71f
                               at /home/yonatan/Code/helix/helix-term/src/application.rs:1078:38
  31:     0x55d78b096f39 - hx::main_impl::{{closure}}::hf3dbaa4901e1f1b8
                               at /home/yonatan/Code/helix/helix-term/src/main.rs:156:53
  32:     0x55d78b0a5e63 - tokio::runtime::park::CachedParkThread::block_on::{{closure}}::h2c0d91489796badb
                               at /home/yonatan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.25.0/src/runtime/park.rs:283:63
  33:     0x55d78b0a577e - tokio::runtime::coop::with_budget::h850cb4b49be4d9ce
                               at /home/yonatan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.25.0/src/runtime/coop.rs:102:5
  34:     0x55d78b0a577e - tokio::runtime::coop::budget::hacf1f321cef44eb5
                               at /home/yonatan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.25.0/src/runtime/coop.rs:68:5
  35:     0x55d78b0a577e - tokio::runtime::park::CachedParkThread::block_on::hbabcdf0a759410f4
                               at /home/yonatan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.25.0/src/runtime/park.rs:283:31
  36:     0x55d78b0aa065 - tokio::runtime::context::BlockingRegionGuard::block_on::h25bd934cf57cecd3
                               at /home/yonatan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.25.0/src/runtime/context.rs:315:13
  37:     0x55d78b06c452 - tokio::runtime::scheduler::multi_thread::MultiThread::block_on::hfd3f01d1f183e754
                               at /home/yonatan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.25.0/src/runtime/scheduler/multi_thread/mod.rs:66:9
  38:     0x55d78b09286b - tokio::runtime::runtime::Runtime::block_on::h1dc72f65432a32c2
                               at /home/yonatan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.25.0/src/runtime/runtime.rs:284:45
  39:     0x55d78b04c714 - hx::main_impl::h49a4c63e5243805f
                               at /home/yonatan/Code/helix/helix-term/src/main.rs:158:5
  40:     0x55d78b04c56e - hx::main::h99e3163e6c87f793
                               at /home/yonatan/Code/helix/helix-term/src/main.rs:38:21
  41:     0x55d78b03ce8b - core::ops::function::FnOnce::call_once::h21a06a7d98bf5c1c
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/ops/function.rs:507:5
  42:     0x55d78b06c26e - std::sys_common::backtrace::__rust_begin_short_backtrace::h1c73c98749fe2ae2
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/sys_common/backtrace.rs:121:18
  43:     0x55d78b0a60a1 - std::rt::lang_start::{{closure}}::h98b81fc1082f200a
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/rt.rs:166:18
  44:     0x55d78c9ff28c - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::h2e730c3fa855d1cd
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/ops/function.rs:606:13
  45:     0x55d78c9ff28c - std::panicking::try::do_call::h2452817705661d69
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:483:40
  46:     0x55d78c9ff28c - std::panicking::try::had9be61d7261a8f5
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:447:19
  47:     0x55d78c9ff28c - std::panic::catch_unwind::h0e0ee12d5e0e8c35
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panic.rs:137:14
  48:     0x55d78c9ff28c - std::rt::lang_start_internal::{{closure}}::hdfa1f1493a65dae1
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/rt.rs:148:48
  49:     0x55d78c9ff28c - std::panicking::try::do_call::h5a2ac6628cbce239
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:483:40
  50:     0x55d78c9ff28c - std::panicking::try::h6b550c44d18ea4fc
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:447:19
  51:     0x55d78c9ff28c - std::panic::catch_unwind::hd574fc8a4fcd921a
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panic.rs:137:14
  52:     0x55d78c9ff28c - std::rt::lang_start_internal::h4189ab0091a05404
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/rt.rs:148:20
  53:     0x55d78b0a607a - std::rt::lang_start::hd283e7d5aa269070
                               at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/rt.rs:165:17
  54:     0x55d78b04c7ce - main
  55:     0x7f185e0e4790 - <unknown>
  56:     0x7f185e0e484a - __libc_start_main
  57:     0x55d78b03ad45 - _start
  58:                0x0 - <unknown>

yonatan8070 avatar Feb 15 '23 11:02 yonatan8070

@wongjiahau don't sweat it

Although it works fine for me. I have a few suggestions

  1. The preview directory should be an option to toggle by users cause I think it's annoying. If we have a key to toggle it, we can just press enter or o to open content in the directory, no need to preview
  2. Agree with @clo4 on mkdir -p, it's a really nice feature in NvimTree also
  3. Space-e should open and close the file explorer. but I understand you wanted to use that to refocus on file explorer
  4. Agree with @clo4 on indentation, NvimTree indent by 2 columns, so it looks clear, I even think that we won't need the line to show which files/directories belong to which parent directories if it's at least indented by 2 columns image
  5. Symlinked directories cannot be opened or shown that it's symlinked image

Overall, I think this is a good work, let me know if you want me to test it. I will add comment if need be

GervinFung avatar Feb 15 '23 13:02 GervinFung

Hi @archseer, I wanted to seek your recommendation.

Currently, the Tree is a new component that has been written from scratch. This means that to incorporate movements such as j, ge, zz, and n, they would need to be re-coded and re-tested.

I'm concerned about the amount of duplicated effort that might go into this, so I'm considering rendering the tree as text on the Editor instead.

This approach is similar to how Vim/Neovim's file explorer works, where it's an uneditable buffer with keybindings overridden accordingly.

However, the current Editor API doesn't support this functionality, at least not without a workaround.

I was hoping to get your recommendation on whether I should pursue the custom component path or the tree-on-editor path.

Thank you!

wongjiahau avatar Feb 18 '23 10:02 wongjiahau

Hi @archseer, I wanted to seek your recommendation.

Currently, the Tree is a new component that has been written from scratch. This means that to incorporate movements such as j, ge, zz, and n, they would need to be re-coded and re-tested.

I'm concerned about the amount of duplicated effort that might go into this, so I'm considering rendering the tree as text on the Editor instead.

This approach is similar to how Vim/Neovim's file explorer works, where it's an uneditable buffer with keybindings overridden accordingly.

However, the current Editor API doesn't support this functionality, at least not without a workaround.

I was hoping to get your recommendation on whether I should pursue the custom component path or the tree-on-editor path.

Thank you!

Using a custom buffer sounds neat in theory as it also allow things like using multi cursors to delete/add rename files (basically you can edit the file tree and if you save it applies those changes to disk. Emacs does something similar

The problem is that the ui would regress in that case. You would have normal cursors moving around instead of selecting one item at a time. Updating the file tree with a file watcher (and using a. centralized fs cache) would also be a lot harder and in general a lot of special handeling in the normal editor would be necessary which I would rather not introduce.

I also think that most editor commands (like multicursors) dont actually make too much sense in a file browser. You probably just need a couple bindings that you can mostly copy from the picker (plus some additions for expanding/collapsing the file tree). That also makes the tree view eadier to reuse for ither things that definitly shkuld be componenets.

Overall I think implementing the treeview as an ui componenent like done here is the right approach. There shoudlnt be too many keybindings and those that do exist should be closer to the picker in their logic rather than the editor.

pascalkuthe avatar Feb 18 '23 11:02 pascalkuthe

Changes since the last comment

  • [x] feat(tree): Sticky ancestors (my favourite feature, no need to scroll to top to find out the current folder(s))
  • [x] style(tree): increase indentation
  • [x] key(explorer): Change '[' to "go to previous root"
  • [x] key(explore): Change 'B' to "go to parent"
  • [x] feat(tree): Use C-o/C-i for jump backward/forward
  • [x] style(explorer): On focus indication
  • [x] feat(explorer): Support creating files and folder and the same time (mkdir -p)
  • [x] test: Add unit test for TreeView and Explorer
  • [x] feat(tree): n/N wrap around
  • [x] fix(filter): crash
  • [x] fix(explorer/preview): panic if not tall enough
  • [x] fix(explorer/preview): content not sorted
  • [x] key(tree): bind "o" to open/close file/folder
  • [x] key(tree): bind "C-n/C-p" to up/down
  • [x] key(explorer): bind "="/"_" to zoom-in/zoom-out
  • [x] refactor(explorer, tree): remove unwrap and expect
  • [x] feat(explorer): toggle preview

This is ready to merge if there are no more bugs.

Thanks for the feedback guys!

wongjiahau avatar Feb 25 '23 05:02 wongjiahau

Feedback:

  • The icons look really good! I really like the arrow aesthetic
  • Hitting : does nothing while the file tree is focused, it wasn't intuitive to me that I had to focus an editor pane first
  • It's hard to tell when the explorer is focused
    • I don't know what I'd do about this, it seems like something that can be left to a follow-up PR
  • I don't think the help-menu (opened with ?) should be persistent once you've toggled it on
    • Instead, should have to toggle it on each time you want to see the help
    • The reason I bring this up: When you open the help menu then un-focus the explorer, it disappears. If you then open the explorer again with space-e, it doesn't look like the space shortcut menu has disappeared. And because of the point above (hard to tell when focused), it doesn't look like anything has happened
  • I'd love y to yank the path of the highlighted item, and likewise if possible space-y to yank it to the system clipboard
    • Not sure if this would yank the absolute path or the path relative to the cwd. I think relative for y and absolute for Y?
    • The space menu isn't implemented for the explorer, not sure how easy that would be to add
    • Like above, this can probably be done in a follow up
  • Being able to open a file in a new split would be amazing
    • Another good candidate for a follow-up
  • Can't un-focus the explorer the same way you focus it, ie. with space-e
    • It's not critical, but it's a good pattern to follow in UX design
  • Not clear how to close explorer when it's not focused
    • E.g. navigate to a file, hit enter, now I want to close the explorer... I have to focus the explorer to do this?
    • It makes sense to not overload space-e by also closing the explorer when it's open. Having it only focus it (or toggle when not visible) is intuitive
    • I think space-x seems fitting, x is usually used for closing things, and it sounds like the first part of explorer. I could definitely be convinced otherwise though
      • Likewise if this is implemented, having the same binding in the explorer would make sense

I really love what you've done with this @wongjiahau. It looks great and works great -- I'd be happy to use it as it is now! Once more, thank you for taking on such a huge endeavor, and sorry about my nitpicky feedback! :)

clo4 avatar Feb 26 '23 06:02 clo4

Incredible work @wongjiahau. Can't wait until this is merged!

cor avatar Feb 27 '23 14:02 cor

Just switched to this pr and I have small suggestions.

  1. It would be nice to ignore hidden files somehow
  2. I feel fuzzy search with feedback from explorer would be nice (maybe as additional filter option?) EDIT. on second thought probably most people won't like duplicating functionality of fuzzy search in explorer.

Great work on the pr :-)

nxy7 avatar Feb 27 '23 15:02 nxy7

How can I successfully this PR on Ubuntu 20.04? The successful test appear to be using rust 1.63 but for me I get

error: failed to compile `helix-term v0.6.0 (/home/johalun/dev/helix/helix-term)`, intermediate artifacts can be found at `/home/johalun/.cargo/target`

Caused by:
  package `git-traverse v0.22.2` cannot be built because it requires rustc 1.64 or newer, while the currently active rustc version is 1.63.0

For newer Rust I get

error[E0282]: type annotations needed
   --> /home/johalun/.cargo/registry/src/github.com-1ecc6299db9ec823/git-glob-0.5.4/src/wildmatch.rs:248:62
    |
248 | ...                   let class = &pattern.as_ref()[p_idx + 2..closing_bracket_idx - 1];
    |                                            ^^^^^^
    |
help: try using a fully qualified path to specify the expected types
    |
248 |                                         let class = &<BStr as AsRef<T>>::as_ref(pattern)[p_idx + 2..closing_bracket_idx - 1];
    |                                                      +++++++++++++++++++++++++++       ~

johalun avatar Feb 27 '23 19:02 johalun

@johalun make sure to build with locked: cargo install --locked --path helix-term

cd-a avatar Feb 27 '23 20:02 cd-a

@cd-a That did it. Thanks!

johalun avatar Feb 27 '23 20:02 johalun

Very nice improvements. I like it 👍🏻

Edit: Can I configure the explorer to be open from start?

johalun avatar Feb 27 '23 20:02 johalun

Bug found when using explorer in non-popup mode:

  • Explorer does not update the active file until I focus it with shift e after I changed file using the file or buffer picker.

johalun avatar Feb 27 '23 23:02 johalun

After few days of usage I can tell that the feature looks great. Thank you

I drop some notes about the things that IMO can be improved:

  • Show [FILTER] and [SEARCH] elements only during a filter or search "session" (that is, when the user press the corresponding keybindings). Clean and close immediately after search (currently the last search string remains in the search prompt).
  • Recursive search. Search should be performed recursively. The search should eventually expand the folders to show the match. Expand only the first match, then expand the next when the user press n.
  • In the explorer's help (?) is missing the key to jump back into the editor. I.e. the action provided by Esc.
  • I don't find of any utility the preview feature (but this is just my opinion 🤔)
  • Typo in the explorer help for the d keybinding. This is not limited to file deletion, but can remove folders as well.
  • Currently, there are two separate options to add a file (a) and a folder (A). To prevent to have options explosion IMO may be better to have a unique a to add both files and dirs (it is also more coherent with the 'd' and 'r' options that are used to rename both files and dirs). To add a dir the new string should be terminated by a /. That is:
    • /file/system/path/hello -> adds a new file named hello
    • /file/system/path/hello/ -> adds a new dir named hello IIRC this is the way most of the file tree plugins work in vim/neovim
  • When deleting a file the confirmation plugin pops up. And asks you to press y or n followed by enter. IMO is sufficient to just press y or n (enter is not necessary). Is this even possible?
  • Would be super nice to be able to perform the same action on multiple files/dirs. For example select multiple files using space bar and then press 'd' to delete all of them in one shot.

davxy avatar Mar 01 '23 08:03 davxy

I think I found a bug, every character I enter is entered twice. I do not have this issue cloning from the main branch, which is why I post it here.

https://user-images.githubusercontent.com/98265679/222495126-d869f166-f749-4099-bfe9-975608a4b833.mp4

Cerabbite avatar Mar 02 '23 16:03 Cerabbite

That was just fixed in master so it is probably not merged into this pr yet

gabydd avatar Mar 02 '23 17:03 gabydd

First I really love this PR, one step closer to being able to use this editor daily. Thank you for your work.

Some of my thoughts:

  • l, right Expand folder, I expected h, left to do the opposite - fold and jump to parent folder
  • Would be nice to use tab key to toggle expand/fold folder, on file it could show a preview of the file
  • I have a hard time jumping between explorer and editor. I expected to use c-w-h|l the same as jumping between windows, but I am used to that from neovim, so maybe it isn't a good idea.
  • space-e focus explorer, I think it is a good idea to unfocus or close with the same keybind
  • Search and filter to be more consistent with helix native search, ideally the same position and style
  • Help section:
    • ~~Add file/folder~~ Create
    • ~~Rename file/folder~~ Rename
    • ~~Delete file~~ Delete
    • Is it necessary to show help that is same in the whole editor? For example n, N, zz, zt, zb,gg, ge, gh, gl
  • Syntax highlighting is not working in the preview

Overall I am really happy with how it looks.

mariansimecek avatar Mar 02 '23 21:03 mariansimecek

@wongjiahau played around with this and I just wanted to note that I find the preview feature to be incredibly helpful! Super nice for quickly understanding the structure of a project 👍🏻

cor avatar Mar 06 '23 16:03 cor