nnn.vim icon indicating copy to clipboard operation
nnn.vim copied to clipboard

`:NnnExplorer` bugs and additional features

Open mcchrish opened this issue 2 years ago • 33 comments

  • [x] :NnnExplorer should toggle the nnn window instead of opening a new window. #138
  • [x] Each tab should only have one explorer window. #138
  • [ ] Nice to have: option to have the same nnn buffer in all tabs.
  • [x] Nice to have: option to change window background when not in focus. Sometimes it's hard to tell where the focus is. #135
  • [ ] Set only bufhidden=wipe to buflisted to nnn picker buffer. Might be useful to still list the explorer buffer specially for navigation.

mcchrish avatar Oct 22 '21 06:10 mcchrish

:NnnExplorer should toggle the nnn window instead of opening a new window.

I think the same behavior should apply to :NnnPicker as well, it's what I did for nnn.nvim at least.

luukvbaal avatar Oct 22 '21 11:10 luukvbaal

  • Nice to have: option to have the same nnn buffer in all tabs.

I thought about it but not sure if that's possible. On NERDTree they just ask you to create a new buffer for each tab. https://github.com/preservim/nerdtree#can-i-have-the-same-nerdtree-on-every-tab-automatically

All the other points seems reasonable.

Also I noticed that if multiple explorer window is opened, then only the last one's fifo is getting deleted/cleaned. So you can add that to the list of bugs.

N-R-K avatar Oct 22 '21 12:10 N-R-K

Nice to have: option to have the same nnn buffer in all tabs.

In nnn.nvim this was default, https://github.com/luukvbaal/nnn.nvim/commit/d4f5763da0dc4852b2d7c9449600383adf5f891d adds the option.

luukvbaal avatar Oct 22 '21 12:10 luukvbaal

One more bug would be this: https://github.com/mcchrish/nnn.vim/pull/123#issuecomment-947802479 explorer apparently behaves badly with minibufexplorer plugin.

One other problem, in nnn.vim in general not just explorer mode, is this https://github.com/mcchrish/nnn.vim/pull/123#issuecomment-947810428 , the plugin AutoComplPop can end up renaming files due to some autocmd.

@mcchrish how do you suggest we deal with these type of compatibility problems? Do we try to fix it on our end or just add it to the docs and expect users to add the fixes to their vimrc?

Since there's thousands of vim plugins, trying to fix these compatibility issues can easily bloat up the plugin code. IMO it makes more sense to instruct the users to apply the fixes that they need instead.

N-R-K avatar Oct 22 '21 12:10 N-R-K

Do we try to fix it on our end or just add it to the docs and expect users to add the fixes to their vimrc?

Give it a shot but don't spend too much time. I don't prefer devs working on their free time spending unreasonable effort on my projects. Whatever you guys do already is much more than enough. @luukvbaal knows it well. ;)

jarun avatar Oct 22 '21 15:10 jarun

I don't prefer devs working on their free time spending unreasonable effort on my projects.

Since I use AutoComplPop myself I was going to fix it anyways. So far I got a rough idea on what to do (setup an autocmd that will disable AutoComplPop when entering nnn buffer and enable it back when leaving the buffer), just need to translate that into vimscript. I just wanted to know weather we want to add the fix to the plugin or add it to the docs.

For the minibufexplorer problem, I'll look into it when I got nothing else to do.

N-R-K avatar Oct 22 '21 16:10 N-R-K

Just hit this with latest:

Error detected while processing function nnn#explorer:
line   22:
E730: Using a List as a String
E484: Can't open file /tmp/vprM9ID/1
Press ENTER or type command to continue

jarun avatar Oct 22 '21 16:10 jarun

Pulled the latest master. I get the same on vim. I think it's due to this commit https://github.com/mcchrish/nnn.vim/commit/cf3e27c138a7cde9c674795cc3bb822395fc5e56

N-R-K avatar Oct 22 '21 17:10 N-R-K

This should fix it,

diff --git a/autoload/nnn.vim b/autoload/nnn.vim
index 6164672..b3835de 100644
--- a/autoload/nnn.vim
+++ b/autoload/nnn.vim
@@ -386,7 +386,7 @@ function! nnn#explorer(...) abort
     let l:On_exit = s:explorer_create_on_exit_callback(l:opts)
 
     " create the fifo ourselves since otherwise nnn might not create it on time
-    call system(['mkfifo', s:explorer_fifo])
+    call system('mkfifo '.shellescape(s:explorer_fifo))
     let l:opts.term = s:build_window(l:layout, { 'cmd': l:cmd, 'on_exit': l:On_exit })
     let b:tbuf = l:opts.term

@jarun I'll open a PR if you can confirm it works.

N-R-K avatar Oct 22 '21 17:10 N-R-K

Yes it works fine. :+1:

jarun avatar Oct 22 '21 17:10 jarun

Another problem:

  • open any file in vim
  • open explorer
  • try to exit with :wqa

I see:

E948: Job still running

jarun avatar Oct 22 '21 17:10 jarun

Another problem:

* open any file in vim

* open explorer

* try to exit with `:wqa`

I see:

E948: Job still running

Hmm, not sure how to fix that. We're already sending SIGTERM, sending SIGKILL doesn't seem to help either.

Looks like nnn.nvim has this problem with :wqa as well. @luukvbaal

N-R-K avatar Oct 22 '21 18:10 N-R-K

Didn't run into this when I tested it before but perhaps there has been a regression.

luukvbaal avatar Oct 22 '21 18:10 luukvbaal

I think I was missing the w in :wqa when I tested this earlier.

This seems to be a general issue with terminal buffers in neovim: https://github.com/neovim/neovim/issues/14061 https://github.com/neovim/neovim/issues/13549. Not sure about vim status.

luukvbaal avatar Oct 22 '21 20:10 luukvbaal

Didn't run into this when I tested it before but perhaps there has been a regression.

I think it is. Because even if I open a new file and :wqa I see this. @N-R-K can you step back a few steps in nnn.vim and check where this got introduced?

jarun avatar Oct 22 '21 21:10 jarun

can you step back a few steps in nnn.vim and check where this got introduced?

I don't think this is a regression. The same issue happens if you try to :wqa with Picker open as well.

This seems to be a general issue with terminal buffers in neovim: neovim/neovim#14061 neovim/neovim#13549. Not sure about vim status.

Seems to be an inherent problem with vim too.

N-R-K avatar Oct 22 '21 22:10 N-R-K

OK. Any workarounds available?

jarun avatar Oct 22 '21 22:10 jarun

OK. Any workarounds available?

:w and then :qa should work.

N-R-K avatar Oct 22 '21 22:10 N-R-K

Thanks!

jarun avatar Oct 22 '21 22:10 jarun

@mcchrish how do you suggest we deal with these type of compatibility problems? Do we try to fix it on our end or just add it to the docs and expect users to add the fixes to their vimrc?

Since there's thousands of vim plugins, trying to fix these compatibility issues can easily bloat up the plugin code. IMO it makes more sense to instruct the users to apply the fixes that they need instead.

I agree it will add bloat to the plugin. If the fix requires a specific code snippet or plugin configuration, then the fix should be in the user vimrc. It probably deserves a mention in README or a pinned issue if the plugin has a lot of users.

mcchrish avatar Oct 22 '21 22:10 mcchrish

Added a troubleshooting section with the AutoComplPop fix. #132

N-R-K avatar Oct 23 '21 13:10 N-R-K

@jarun Checked out minibufexpl, I think the behavior of opening on a new buffer is expected. We don't want to wipe the current buffer in case there's unsaved changes. nnn.nvim is doing the same thing.

N-R-K avatar Oct 23 '21 14:10 N-R-K

@jarun Checked out minibufexpl, I think the behavior of opening on a new buffer is expected. We don't want to wipe the current buffer in case there's unsaved changes. nnn.nvim is doing the same thing.

Hmm, wait. For some reason it's opening the new buffer and replacing the current nnn buffer. That's not expected. Also that wasn't happening when I tested before, but now it is.

N-R-K avatar Oct 23 '21 17:10 N-R-K

Aright, so the bug only happens if you invoke :NnnExplorer while minibufexpl isn't opened. @jarun you can add the following to your vimrc as a workaround:

let g:miniBufExplBuffersNeeded = 1

N-R-K avatar Oct 23 '21 17:10 N-R-K

Works like a charm! Please document.

jarun avatar Oct 23 '21 17:10 jarun

Hmm, the minibufexpl problem doesn't occur on nnn.nvim, I might look into why that is. However the file rename due to AutoComplPop happens there as well, @luukvbaal you may want to document that.

N-R-K avatar Oct 23 '21 18:10 N-R-K

Took this from NERDTree but removed a check b:NERDTree.isTabTree(). Seems to be working fine.

@mcchrish Do you think this is okay or are there some edge case?

" Exit Vim if NnnExplorer is the only window remaining in the only tab.
autocmd BufEnter * if tabpagenr('$') == 1 && winnr('$') == 1 && exists('b:nnn_ftplugin') | quit | endif

" Close the tab if NnnExplorer is the only window remaining in it.
autocmd BufEnter * if winnr('$') == 1 && exists('b:nnn_ftplugin') | quit | endif

N-R-K avatar Oct 29 '21 04:10 N-R-K

@N-R-K the code looks fine. I'm not sure I'd want this behavior though. But if some wants it, maybe have a config to turn it on.

You can also check for the file type instead of b:nnn_ftplugin: &filetype ==# 'nnn'

mcchrish avatar Oct 30 '21 07:10 mcchrish

@mcchrish I'm thinking about adding it to the docs similar to NERDTree.

You can also check for the file type instead of b:nnn_ftplugin: &filetype ==# 'nnn'

Noted. Will open a PR later.

N-R-K avatar Oct 30 '21 07:10 N-R-K

  • [ ] :NnnExplorer should toggle the nnn window instead of opening a new window.

Any plans on how to implement this? I was trying out something like this, but this doesn't work well when called form a different tab.

diff --git a/autoload/nnn.vim b/autoload/nnn.vim
index a4929da..70844aa 100644
--- a/autoload/nnn.vim
+++ b/autoload/nnn.vim
@@ -3,6 +3,7 @@ let s:action = ''
 let s:nnn_conf_dir = (!empty($XDG_CONFIG_HOME) ? $XDG_CONFIG_HOME : $HOME.'/.config') . '/nnn'
 " The fifo used by the persistent explorer
 let s:explorer_fifo = ""
+let s:explorer_winid = 0
 
 let s:local_ses = 'nnn_vim_'
 " Add timestamp for convenience
@@ -364,6 +365,11 @@ function! nnn#pick(...) abort
 endfunction
 
 function! nnn#explorer(...) abort
+    if s:explorer_winid > 0
+        call win_execute(s:explorer_winid, 'close!')
+        let s:explorer_winid = 0
+        return
+    endif
     let l:directory = get(a:, 1, '')
     let l:default_opts = { 'edit': 'edit' }
     let l:opts = extend(l:default_opts, get(a:, 2, {}))
@@ -388,6 +394,7 @@ function! nnn#explorer(...) abort
     call system('mkfifo '.shellescape(s:explorer_fifo))
     let l:opts.term = s:build_window(l:layout, { 'cmd': l:cmd, 'on_exit': l:On_exit })
     let b:tbuf = l:opts.term
+    let s:explorer_winid = l:opts.term.winhandle
 
     call s:explorer_job()

N-R-K avatar Oct 31 '21 15:10 N-R-K