[vim] Don't start extra process at opening popup

[ichizok]

I propose to use pty-buffer instead of unnecessary shell process when opening popup window.

[junegunn]

/cc @lacygoill

[lacygoill]

If a terminal popup is already open when an fzf command is invoked, when leaving the fzf popup and getting back to the original popup, E994 is raised, and the terminal buffer is not wiped out; it remains present in the output of :ls!:

6  h?  "!pty [active]"                line 1

[ichizok]

@lacygoill Hm,, we can fail pty cleanup.

I devised the another plan, how is this?

--- a/plugin/fzf.vim
+++ b/plugin/fzf.vim
@@ -648,6 +648,7 @@ function! s:split(dict)
   \ 'left':  ['vertical topleft', 'vertical resize', &columns],
   \ 'right': ['vertical botright', 'vertical resize', &columns] }
   let ppos = s:getpos()
+  let is_popup = v:false
   try
     if s:present(a:dict, 'window')
       if type(a:dict.window) == type({})
@@ -655,6 +656,7 @@ function! s:split(dict)
           throw 'Vim 8.2.191 or later is required for pop-up window'
         end
         call s:popup(a:dict.window)
+        let is_popup = v:true
       else
         execute 'keepalt' a:dict.window
       endif
@@ -676,7 +678,7 @@ function! s:split(dict)
         endif
       endfor
     endif
-    return [ppos, { '&l:wfw': &l:wfw, '&l:wfh': &l:wfh }]
+    return [ppos, { '&l:wfw': &l:wfw, '&l:wfh': &l:wfh }, is_popup]
   finally
     setlocal winfixwidth winfixheight
   endtry
@@ -685,7 +687,7 @@ endfunction
 function! s:execute_term(dict, command, temps) abort
   let winrest = winrestcmd()
   let pbuf = bufnr('')
-  let [ppos, winopts] = s:split(a:dict)
+  let [ppos, winopts, is_popup] = s:split(a:dict)
   call s:use_sh()
   let b:fzf = a:dict
   let fzf = { 'buf': bufnr(''), 'pbuf': pbuf, 'ppos': ppos, 'dict': a:dict, 'temps': a:temps,
@@ -752,7 +754,13 @@ function! s:execute_term(dict, command, temps) abort
       if !len(&bufhidden)
         setlocal bufhidden=hide
       endif
-      let fzf.buf = term_start([&shell, &shellcmdflag, command], {'curwin': 1, 'exit_cb': function(fzf.on_exit)})
+      let term_opts = {'exit_cb': function(fzf.on_exit)}
+      if is_popup
+        let term_opts.hidden = 1
+      else
+        let term_opts.curwin = 1
+      endif
+      let fzf.buf = term_start([&shell, &shellcmdflag, command], term_opts)
       if !has('patch-8.0.1261') && !has('nvim') && !s:is_win
         call term_wait(fzf.buf, 20)
       endif
@@ -824,23 +832,22 @@ if has('nvim')
 else
   function! s:create_popup(hl, opts) abort
     let is_frame = has_key(a:opts, 'border')
-    let buf = is_frame ? '' : term_start(&shell, #{hidden: 1, term_finish: 'close'})
-    let id = popup_create(buf, #{
+    let s:popup_create = {buf -> popup_create(buf, #{
       \ line: a:opts.row,
       \ col: a:opts.col,
       \ minwidth: a:opts.width,
       \ minheight: a:opts.height,
       \ zindex: 50 - is_frame,
-    \ })
-
+    \ })}
     if is_frame
+      let id = s:popup_create('')
       call setwinvar(id, '&wincolor', a:hl)
       call setbufline(winbufnr(id), 1, a:opts.border)
       execute 'autocmd BufWipeout * ++once call popup_close('..id..')'
+      return winbufnr(id)
     else
-      execute 'autocmd BufWipeout * ++once call term_sendkeys('..buf..', "exit\<CR>")'
+      autocmd TerminalOpen * ++once call s:popup_create(str2nr(expand('<abuf>')))
     endif
-    return winbufnr(id)
   endfunction
 endif

[lacygoill]

Nice, it indeed fixes the issue. Unfortunately, it creates other issues.
With this patch, when I run :FzfHistory provided by the fzf.vim plugin, this error is raised:

Error detected while processing FileType Autocommands for "fzf":
E31: No such mapping
Error detected while processing function <SNR>52_history:
line    7:
E171: Missing :endif

The same issue affects other commands like :FzfColors or :FzfMaps:

Error detected while processing FileType Autocommands for "fzf":
E31: No such mapping

Note that these errors are raised, even if you don't have a popup terminal open before running these commands.

---

I agree with you, it seems weird for an unused shell process to be started. But I don't think there's an easy way to get rid of it now; unless a deeper refactoring is performed. Besides, I'm not sure the added complexity is worth it.

Maybe the best fix would be your first patch, which is much simpler. But for it to work reliably, :bw would need to be allowed when an existing popup terminal is open. Right now it's forbidden. You could try to delay the command via some one-shot autocmd listening to some event(s) but it's tricky, and probably not completely reliable.

[ichizok]

I cannot reproduce the error... please provide an example (vimrc).

[lacygoill]

Here is a minimal vimrc:

set rtp-=~/.vim
set rtp-=~/.vim/after
set rtp^=~/.fzf
set rtp^=~/.vim/plugged/fzf.vim
au TerminalWinOpen * tno <buffer><nowait> <esc><esc> <c-\><c-n>
au FileType fzf sil tunmap <buffer> <esc><esc>
let g:fzf_layout = {
   \ 'window': {
   \     'width': 0.9,
   \     'height': 0.6,
   \ }}

Write the code in /tmp/vimrc, then start Vim with this shell command:

vim -Nu /tmp/vimrc

Finally, run this Ex command (provided by the fzf.vim plugin):

:History

It raises this error:

Error detected while processing FileType Autocommands for "fzf":
E31: No such mapping
Error detected while processing function <SNR>3_history:
line    7:
E171: Missing :endif

---

You may wonder what is the purpose of these autocmds:

au TerminalWinOpen * tno <buffer><nowait> <esc><esc> <c-\><c-n>
au FileType fzf tunmap <buffer> <esc><esc>

The first one installs a mapping on <esc><esc> to quit Terminal-Job mode and get back to Terminal-Normal mode.
The lhs doesn't use a single <esc>, because the resulting mapping would cause Vim to wrongly remap an Escape produced by a special key (like F1-F12 keys or arrow keys), which can lead to various unexpected behaviors. See here for an example.

I once asked @leonerd on the #vim irc channel what was the cause of such issues, and here's what they explained to me:

In vim- raw bytes arrive from the terminal, go into the map engine (which just looks to see if the
headofqueue bytes match any of its mappings, and replace them if so), then bytes come out directly into the command system
Whereas in nvim, raw bytes go into the termkey instance, which turns them into keys, and those keys are what goes into the map engine
So the map engine is never confused by the individual component bytes that formed those keypresses
Just like it isn't confused by the individual bytes that came across the X11 unix socket when in gvim mode, or.. any other stupid crazy things that wouldn't be possible if it was actually sensible

Until Vim's keyboard input system is improved, the best workaround I have found so far is to double the <esc> in the lhs of the mapping.

Now, for the second autocmd:

au FileType fzf tunmap <buffer> <esc><esc>

Its purpose is to remove the previous mapping in fzf buffers (which are terminal buffers). It's not needed there, and if it's installed you can't close the fzf window by pressing Escape anymore.

I think the reason why the error is raised after I apply your patch, is because for some reason it doesn't trigger TerminalWinOpen anymore, which causes the <esc><esc> mapping not to be installed anymore, which causes :tunmap to fail.

I guess I could fix it by simply using the :silent! modifier or just by removing the 2nd autocmd, but I wonder why TerminalWinOpen is not triggered anymore. Here is the list of events fired before the patch is applied (when :History is run). And here is the list of events fired after the patch is applied.

Several events are no longer fired, including:

BufEnter
BufHidden
BufLeave
BufWinEnter
BufWinLeave
CursorMoved
FileType
TerminalWinOpen
WinEnter
WinLeave

This could cause issues if a user relies on one of them to customize an fzf buffer (in particular FileType).

[ichizok]

Several events are no longer fired, including:

BufEnter
BufHidden
BufLeave
BufWinEnter
BufWinLeave
CursorMoved
FileType
TerminalWinOpen
WinEnter
WinLeave

BufEnter, BufHidden, BufLeave, BufWinEnter, BufWinLeave, WinEnter, WinLeave
I think: using these events bases on the knowledge of internal implementation (calling term_start() twice), thus do not need to keep their supports, which are undocumented.

FileType
The first error occurs in FileType fzf event, so then should be firing.

Error detected while processing FileType Autocommands for "fzf":
E31: No such mapping

CursorMoved
Also should be firing, but I'm not sure.

TerminalWinEnter
I also do not think need to keep, but we can use the following patch to fire the event by calling doautocmd explicitly after term_start(). (this should keep the order of events)

(part of diff)

@@ -752,8 +754,17 @@ function! s:execute_term(dict, command, temps) abort
       if !len(&bufhidden)
         setlocal bufhidden=hide
       endif
-      let fzf.buf = term_start([&shell, &shellcmdflag, command], {'curwin': 1, 'exit_cb': function(fzf.on_exit)})
-      if !has('patch-8.0.1261') && !has('nvim') && !s:is_win
+      let term_opts = {'exit_cb': function(fzf.on_exit)}
+      if is_popup
+        let term_opts.hidden = 1
+      else
+        let term_opts.curwin = 1
+      endif
+      let fzf.buf = term_start([&shell, &shellcmdflag, command], term_opts)
+      if is_popup
+        doautocmd TerminalWinOpen
+      endif
+      if !has('patch-8.0.1261') && !s:is_win
         call term_wait(fzf.buf, 20)
       endif
     endif

[lacygoill]

It works, but it creates an issue in Nvim when g:fzf_layout is not set:

Error detected while processing function <SNR>53_history[7]..fzf#vim#history[1]..<SNR>163_fzf[18]..fzf#run[58]..<SNR>52
_execute_term:
line    3:
E688: More targets than List items
Error detected while processing function <SNR>53_history:
line    7:
E171: Missing :endif

This patch seems to fix it:

diff --git a/plugin/fzf.vim b/plugin/fzf.vim
index 356ceeb..24dd0b2 100644
--- a/plugin/fzf.vim
+++ b/plugin/fzf.vim
@@ -687,7 +687,11 @@ endfunction
 function! s:execute_term(dict, command, temps) abort
   let winrest = winrestcmd()
   let pbuf = bufnr('')
-  let [ppos, winopts, is_popup] = s:split(a:dict)
+  if has('nvim')
+      let [ppos, winopts] = s:split(a:dict)
+  else
+      let [ppos, winopts, is_popup] = s:split(a:dict)
+  endif
   call s:use_sh()
   let b:fzf = a:dict
   let fzf = { 'buf': bufnr(''), 'pbuf': pbuf, 'ppos': ppos, 'dict': a:dict, 'temps': a:temps,

The final patch looks like this:

diff --git a/plugin/fzf.vim b/plugin/fzf.vim
index a84dcab..24dd0b2 100644
--- a/plugin/fzf.vim
+++ b/plugin/fzf.vim
@@ -648,6 +648,7 @@ function! s:split(dict)
   \ 'left':  ['vertical topleft', 'vertical resize', &columns],
   \ 'right': ['vertical botright', 'vertical resize', &columns] }
   let ppos = s:getpos()
+  let is_popup = v:false
   try
     if s:present(a:dict, 'window')
       if type(a:dict.window) == type({})
@@ -655,6 +656,7 @@ function! s:split(dict)
           throw 'Vim 8.2.191 or later is required for pop-up window'
         end
         call s:popup(a:dict.window)
+        let is_popup = v:true
       else
         execute 'keepalt' a:dict.window
       endif
@@ -676,7 +678,7 @@ function! s:split(dict)
         endif
       endfor
     endif
-    return [ppos, { '&l:wfw': &l:wfw, '&l:wfh': &l:wfh }]
+    return [ppos, { '&l:wfw': &l:wfw, '&l:wfh': &l:wfh }, is_popup]
   finally
     setlocal winfixwidth winfixheight
   endtry
@@ -685,7 +687,11 @@ endfunction
 function! s:execute_term(dict, command, temps) abort
   let winrest = winrestcmd()
   let pbuf = bufnr('')
-  let [ppos, winopts] = s:split(a:dict)
+  if has('nvim')
+      let [ppos, winopts] = s:split(a:dict)
+  else
+      let [ppos, winopts, is_popup] = s:split(a:dict)
+  endif
   call s:use_sh()
   let b:fzf = a:dict
   let fzf = { 'buf': bufnr(''), 'pbuf': pbuf, 'ppos': ppos, 'dict': a:dict, 'temps': a:temps,
@@ -752,8 +758,23 @@ function! s:execute_term(dict, command, temps) abort
       if !len(&bufhidden)
         setlocal bufhidden=hide
       endif
-      let fzf.buf = term_start([&shell, &shellcmdflag, command], {'curwin': 1, 'exit_cb': function(fzf.on_exit)})
-      if !has('patch-8.0.1261') && !has('nvim') && !s:is_win
+      let term_opts = {'exit_cb': function(fzf.on_exit)}
+      if is_popup
+        let term_opts.hidden = 1
+      else
+        let term_opts.curwin = 1
+      endif
+      let term_opts = {'exit_cb': function(fzf.on_exit)}
+      if is_popup
+        let term_opts.hidden = 1
+      else
+        let term_opts.curwin = 1
+      endif
+      let fzf.buf = term_start([&shell, &shellcmdflag, command], term_opts)
+      if is_popup
+        doautocmd TerminalWinOpen
+      endif
+      if !has('patch-8.0.1261') && !s:is_win
         call term_wait(fzf.buf, 20)
       endif
     endif
@@ -824,23 +845,22 @@ if has('nvim')
 else
   function! s:create_popup(hl, opts) abort
     let is_frame = has_key(a:opts, 'border')
-    let buf = is_frame ? '' : term_start(&shell, #{hidden: 1, term_finish: 'close'})
-    let id = popup_create(buf, #{
+    let s:popup_create = {buf -> popup_create(buf, #{
       \ line: a:opts.row,
       \ col: a:opts.col,
       \ minwidth: a:opts.width,
       \ minheight: a:opts.height,
       \ zindex: 50 - is_frame,
-    \ })
-
+    \ })}
     if is_frame
+      let id = s:popup_create('')
       call setwinvar(id, '&wincolor', a:hl)
       call setbufline(winbufnr(id), 1, a:opts.border)
       execute 'autocmd BufWipeout * ++once call popup_close('..id..')'
+      return winbufnr(id)
     else
-      execute 'autocmd BufWipeout * ++once call term_sendkeys('..buf..', "exit\<CR>")'
+      autocmd TerminalOpen * ++once call s:popup_create(str2nr(expand('<abuf>')))
     endif
-    return winbufnr(id)
   endfunction
 endif

It looks good to me; I tested it in:

• Vim without setting g:fzf_layout
• Vim setting g:fzf_layout to use a Vim window
• Vim setting g:fzf_layout to use a popup terminal
• NVim without setting g:fzf_layout
• Nvim setting g:fzf_layout to use a Nvim window
• Nvim setting g:fzf_layout to use a float

I didn't find an error in any case. And the implementation looks cleaner.

@ichizok Thank you for your patch. Can you push an updated version which includes your latest changes as well as the last patch to avoid an error in Nvim when g:fzf_layout is not set?

@junegunn When it's ready, I think you'll want to merge this PR because the current implementation is now broken on Vim when fzf is configured to use a popup terminal. The issue is caused by the patch 8.2.0743. You can no longer replace the current terminal buffer with another one. As a result, this worked before the patch:

$ vim -Nu NONE -S <(cat <<'EOF'
    set hidden
    call term_start('sh', #{hidden: 1})->popup_create({})
    call term_start(['sh', '-c', ':'], #{curwin: 1})
EOF
)

But not anymore. Now, the last term_start() raises E863:

E863: Not allowed for a terminal in a popup window

The current implementation does something similar. The new implementation from @ichizok fixes this new issue.

[lacygoill]

Looks like I talked too fast; the patch still causes issues in Nvim. I'm looking into it.

[lacygoill]

Ok, so the previous patch which I posted was wrong; it fixed the case where g:fzf_layout did not include a window key in Nvim, but it also created an issue when g:fzf_layout did include a window key. This new patch seems to fix both cases:

diff --git a/plugin/fzf.vim b/plugin/fzf.vim
index 356ceeb..c35062c 100644
--- a/plugin/fzf.vim
+++ b/plugin/fzf.vim
@@ -674,7 +674,7 @@ function! s:split(dict)
           endif
           execute cmd sz.'new'
           execute resz sz
-          return [ppos, {}]
+          return [ppos, {}, v:false]
         endif
       endfor
     endif

Now, the final patch looks like this:

diff --git a/plugin/fzf.vim b/plugin/fzf.vim
index a84dcab..c35062c 100644
--- a/plugin/fzf.vim
+++ b/plugin/fzf.vim
@@ -648,6 +648,7 @@ function! s:split(dict)
   \ 'left':  ['vertical topleft', 'vertical resize', &columns],
   \ 'right': ['vertical botright', 'vertical resize', &columns] }
   let ppos = s:getpos()
+  let is_popup = v:false
   try
     if s:present(a:dict, 'window')
       if type(a:dict.window) == type({})
@@ -655,6 +656,7 @@ function! s:split(dict)
           throw 'Vim 8.2.191 or later is required for pop-up window'
         end
         call s:popup(a:dict.window)
+        let is_popup = v:true
       else
         execute 'keepalt' a:dict.window
       endif
@@ -672,11 +674,11 @@ function! s:split(dict)
           endif
           execute cmd sz.'new'
           execute resz sz
-          return [ppos, {}]
+          return [ppos, {}, v:false]
         endif
       endfor
     endif
-    return [ppos, { '&l:wfw': &l:wfw, '&l:wfh': &l:wfh }]
+    return [ppos, { '&l:wfw': &l:wfw, '&l:wfh': &l:wfh }, is_popup]
   finally
     setlocal winfixwidth winfixheight
   endtry
@@ -685,7 +687,7 @@ endfunction
 function! s:execute_term(dict, command, temps) abort
   let winrest = winrestcmd()
   let pbuf = bufnr('')
-  let [ppos, winopts] = s:split(a:dict)
+  let [ppos, winopts, is_popup] = s:split(a:dict)
   call s:use_sh()
   let b:fzf = a:dict
   let fzf = { 'buf': bufnr(''), 'pbuf': pbuf, 'ppos': ppos, 'dict': a:dict, 'temps': a:temps,
@@ -752,8 +754,23 @@ function! s:execute_term(dict, command, temps) abort
       if !len(&bufhidden)
         setlocal bufhidden=hide
       endif
-      let fzf.buf = term_start([&shell, &shellcmdflag, command], {'curwin': 1, 'exit_cb': function(fzf.on_exit)})
-      if !has('patch-8.0.1261') && !has('nvim') && !s:is_win
+      let term_opts = {'exit_cb': function(fzf.on_exit)}
+      if is_popup
+        let term_opts.hidden = 1
+      else
+        let term_opts.curwin = 1
+      endif
+      let term_opts = {'exit_cb': function(fzf.on_exit)}
+      if is_popup
+        let term_opts.hidden = 1
+      else
+        let term_opts.curwin = 1
+      endif
+      let fzf.buf = term_start([&shell, &shellcmdflag, command], term_opts)
+      if is_popup
+        doautocmd TerminalWinOpen
+      endif
+      if !has('patch-8.0.1261') && !s:is_win
         call term_wait(fzf.buf, 20)
       endif
     endif
@@ -824,23 +841,22 @@ if has('nvim')
 else
   function! s:create_popup(hl, opts) abort
     let is_frame = has_key(a:opts, 'border')
-    let buf = is_frame ? '' : term_start(&shell, #{hidden: 1, term_finish: 'close'})
-    let id = popup_create(buf, #{
+    let s:popup_create = {buf -> popup_create(buf, #{
       \ line: a:opts.row,
       \ col: a:opts.col,
       \ minwidth: a:opts.width,
       \ minheight: a:opts.height,
       \ zindex: 50 - is_frame,
-    \ })
-
+    \ })}
     if is_frame
+      let id = s:popup_create('')
       call setwinvar(id, '&wincolor', a:hl)
       call setbufline(winbufnr(id), 1, a:opts.border)
       execute 'autocmd BufWipeout * ++once call popup_close('..id..')'
+      return winbufnr(id)
     else
-      execute 'autocmd BufWipeout * ++once call term_sendkeys('..buf..', "exit\<CR>")'
+      autocmd TerminalOpen * ++once call s:popup_create(str2nr(expand('<abuf>')))
     endif
-    return winbufnr(id)
   endfunction
 endif

[lacygoill]

Actually, I don't know whether it matters for Nvim, but the third argument should probably be is_popup instead of v:false.

diff --git a/plugin/fzf.vim b/plugin/fzf.vim
index 356ceeb..c35062c 100644
--- a/plugin/fzf.vim
+++ b/plugin/fzf.vim
@@ -674,7 +674,7 @@ function! s:split(dict)
           endif
           execute cmd sz.'new'
           execute resz sz
-          return [ppos, {}]
+          return [ppos, {}, is_popup]
         endif
       endfor
     endif

I'll keep using the patch for a few days and report any issue I find.

[ichizok]

@lacygoill Thank you for your confirmation. I updated the PR commit.

[junegunn]

Merged, thanks!

[amadeus]

I believe there is a small bug with this commit - where after I use fzf in a popout (using normal vim, not nvim) and then start opening other splits - the window from which I spawned the fzf popup ends up not resizing properly.

[amadeus]

Here's an example video. If I move to the commit before this, then this does not reproduce.

[lacygoill]

@amadeus Try the patch mentioned here.

[amadeus]

@lacygoill this does appear to fix my issue!