mcchrish / nnn.vim

File manager for vim/neovim powered by n³
BSD 2-Clause "Simplified" License
645 stars 24 forks source link

File explorer (a.k.a. persistent picker) #103

Closed mizlan closed 2 years ago

mizlan commented 3 years ago

Not ready to merge yet, but I'd like a review. There are a number of outstanding issues:

  1. If a floating nnn window is used alongside the file explorer, the value of s:tbuf is overwritten and it is borked (not horrible though, still usable).
  2. Entering and leaving the file explorer window will be a common part of a workflow, but normal window commands cannot be used to leave the window because it is in insert mode in the terminal. (For entering the window, I add an autocommand in b3df46d which works great).
  3. I add a 0.5-second delay before I start to "watch" the FIFO since I'm currently relying on nnn to create the FIFO. I could also create the FIFO within the plugin before starting the nnn process.
  4. There is quite a bit of code duplication between nnn#pick() and nnn#explorer() as well as between s:explorer_create_on_exit_callback() and s:switch_back(). Would you like me to extract common parts out into separate functions?

Those are the issues I can think of at the moment, there may very well be more.

mcchrish commented 3 years ago

@mizlan I think it's ok to convert the PR into a draft temporarily if not yet ready.

jarun commented 3 years ago

I have added the label work in progress. @mizlan, please remove when you are done and leave a message for review.

@mcchrish please do not use shiny features like discussions, drafts when there are easier ways to achieve that. Those add workflow learning curve and confusion.

mcchrish commented 3 years ago

@jarun all of that seems pretty subjective.

In the end, it's just a suggestion. Whatever works for the users and contributors I'm cool with it.

jarun commented 3 years ago

all of that seems pretty subjective.

Let's just concentrate on the subject of implementation and fixing issues for now. The other day you started a discussion and we didn't know how to close that. I do not want those distractions. I am happy we are in a much better shape than we were 2 months back. That works best. :)

Also, please add @annagrram as a collaborator. I see I don't have the rights.

mcchrish commented 3 years ago

I add a 0.5-second delay before I start to "watch" the FIFO since I'm currently relying on nnn to create the FIFO. I could also create the FIFO within the plugin before starting the nnn process.

I don't see a problem with that and creating the FIFO maybe better within the plugin if it eliminates the 0.5 sec hack.

mizlan commented 3 years ago

I'm going to first open a PR to add support for multiple instances as discussed here #102 and then rebase this afterwards.

mizlan commented 3 years ago

I don't really have the time right now to add the support for multiple instances, so maybe @mcchrish if you have some time you could help with that. After that it shouldn't be too bad to get this merged.

jarun commented 3 years ago

@mcchrish @mizlan is busy. See if you can complete the implementation.

N-R-K commented 2 years ago

2. Entering and leaving the file explorer window will be a common part of a workflow, but normal window commands cannot be used to leave the window because it is in insert mode in the terminal.

One can use norm <C-w>l in nnn#action to get around that, which we can mention in the docs. So for example, assuming using a left sided explorer, let g:nnn#action = { '<c-l>': 'norm <C-w>l' } will allow ^l to switch back to normal buffer.


With the following patch, this PR works correctly with nnn v4.3 (file explorer requires -F 1 which is only avail in v4.3 so the docs need to be updated).

diff --git a/autoload/nnn.vim b/autoload/nnn.vim
index 82e1b9b..748dd6e 100644
--- a/autoload/nnn.vim
+++ b/autoload/nnn.vim
@@ -390,7 +390,6 @@ endfunction
 function! s:explorer_create_on_exit_callback(opts)
     function! s:callback(id, code, ...) closure
         let l:term_wins = a:opts.term_wins
-        call delete(fnameescape(s:explorer_fifo))
    " same code as in the bottom of s:switch_back()
         try
             if has('nvim')
@@ -449,9 +448,12 @@ function! nnn#explorer(...) abort
     let l:directory = get(a:, 1, '')
     let l:default_opts = { 'edit': 'edit' }
     let l:opts = extend(l:default_opts, get(a:, 2, {}))
-    let s:explorer_fifo = tempname()
+    if !$NNN_FIFO || $NNN_FIFO == ""
+        let $NNN_FIFO = "/tmp/nnn_ex.fifo"
+    endif
+    let s:explorer_fifo = $NNN_FIFO

-    let l:cmd = g:nnn#command.' -X '.shellescape(s:explorer_fifo).' '.(l:directory != '' ? shellescape(l:directory): '')
+    let l:cmd = g:nnn#command.' -F 1'.' '.(l:directory != '' ? shellescape(l:directory): '')

     let l:layout = exists('l:opts.layout') ? l:opts.layout : g:nnn#explorer_layout

Trying to rebase this on top of master creates a bunch of errors which I don't really know how to solve. And since I don't use the file-explorer, I don't have much motivation to figure out either.

Here's a rebased (full) diff that can be applied cleanly ontop of master https://dpaste.com/3CDZJNL65. @mcchrish Can you take a look and see what's causing the errors?

mcchrish commented 2 years ago

One can use norm <C-w>l in nnn#action to get around that, which we can mention in the docs. So for example, assuming using a left sided explorer, let g:nnn#action = { '<c-l>': 'norm <C-w>l' } will allow ^l to switch back to normal buffer.

See https://github.com/mcchrish/nnn.vim/discussions/118#discussioncomment-1439776 for the recommended way for custom mapping. nnn#action is meant for mapping keys to open selections.

I unfortunately don't have time get deep into this but it seems -F is the way forward.

N-R-K commented 2 years ago

See #118 (comment) for the recommended way for custom mapping

That should work as well.

I unfortunately don't have time get deep into this but it seems -F is the way forward.

I guess we'll then have to wait for @mizlan to be free. Even if I manage to solve the current errors, I might not be available for fixing any issues that might come up with the file explorer.

jarun commented 2 years ago

Closing the PR as we have a new one on latest now.