fix: reload before unhide buffers/tabs pickers

[phanen]

After da9a089 480e29c
Actions like ctrl-x may throw an error since CTX updated.

This PR

• reload all CTX include buflist for buffers
• force a reload by a dummy reload bind when unhide

[ibhagwan]

After da9a089
Actions like ctrl-x may throw an error since CTX updated.

Interesting, I don’t see the connection to the changes in the commit to reload actions.

Tysm @phanen for yet another PR :)

Can’t say I’m super excited about this as it feels a bit “hacky” having a dummy reload bind, I’m trying to think is there another approach to this with an fzf event? Maybe a resize bind triggers on unhide? Maybe with a opts._run_once which gets cleared after a CTX reload?

[ibhagwan]

Btw, there is a better solution but it requires a big refactor and also to drop support for skim, I’ve been thinking for a while of using a fzf --listen, have a few processes of fzf running and communicate with fzf using libcurl bindings or luasocket with http requests, this gives us most flexibility and also eliminates the process spawn time on windows (slow).

With listen we can post a “reload” request anytime over the HTTP socket not needing a bind .

[ibhagwan]

@phanen, checkout main...pr_1871, I still wasn't able to make it work but I feel this is the right direction, I'm trying to hijack the resize event and then using a variable as I mentioned at the previous comments to perform a "oneshot" reload.

The resize however doesn't trigger on unhide so I've been trying to trigger a resize with SIGWINCH (signal 28) but it seems to do nothing when sent to fzf (even with kill -28 <fzf pid?), perhaps there's another signal that can work with fzf but I didn't have time as I had to leave.

I will attempt to solve this later.

[phanen]

Interesting, I don’t see the connection to the changes in the commit to reload actions.

Sorry not da9a089, it's 480e29c.

I’ve been thinking for a while of using a fzf --listen

Yes, with this would be easier as just curl a reload action and possible to avoid killing a fzf process, and also elimate tons of hack in current fzf-lua (e.g. on windows there's no SIGWINCH 28? so fzf's `resize' don't work on it https://github.com/junegunn/fzf/blob/10cbac20f96de35acf272ddc4a998868c5694bd9/src/terminal_windows.go#L9).
I once wrote a demo https://github.com/phanen/fzf.lua, maybe fzf-lua first need some layer like this to replace https://github.com/vijaymarupudi/nvim-fzf.git.

it seems to do nothing when sent to fzf (even with kill -28 <fzf

It work for me? At least it print sent 17445 28 when I unhide.

[ibhagwan]

Yes, with this would be easier as just curl a reload action and possible to avoid killing a fzf process, and also elimate tons of hack in current fzf-lua (e.g. on windows there's no SIGWINCH 28? so fzf's `resize' don't work on it

Maybe it’s not that critical but I hate to rely on additional binaries being is installed, that’s why I prefer a native lua HTTP post library but luasocket is yet another requirement so I’m also not loving it.

Windows not having sig 28 I can live with, windows already has loads of limitations and doesn’t really work well with hide, for example git_status plus hide on windows doesn’t work due to “command too long` (too many reload + execute silent actions).

I’m still not sure why sit 28 doesn’t work with fzf on my Linux, to be explored.

I once wrote a demo https://github.com/phanen/fzf.lua

Interesting we had the same thought :)

maybe fzf-lua first need some layer like this to replace https://github.com/vijaymarupudi/nvim-fzf.git.

My idea was an Fzf class/object, each new instance with their own listen address (+ auth token so “safe”) so we’re not limited to one fzf process, that the user can instantiate and attach to any window, that way you could run say :FzfLua live_grep instance=1 and FzfLua files instance=2 and then you have a persistent live grep/files, when you run :FzfLua resume instance=2 you’re back on the files instance and can guarantee it won’t get overwritten by the next :FzfLua …, we can also have “sticky” fzf-lua windows this way.

A few concerns with the new approach:

1. Increased requirements (curl/lib/etc, unless we can find native simple lua HTTP post lib)
2. We have to drop skim
3. I’m unsure if the HTTP API is sufficient to do everything fzf-lua needs yet (junegunn keeps adding new methods so that’s a plus)
4. FzfLua has been very stable for a while now, this will be a lot of work/testing all while being in the “nice to have” category (not really user driven because we’re “missing” something)

On the plus side this will simplify the code significantly and we can create a proper API and code structure from scratch as opposed to the sometimes not so organized building blocks we currently have due to fzf-lua being developed for a long time incrementally, definitely doesn’t help that I’m also busy in life and other interesting projects so have to find the right motivation for such a big rewrite.

[phanen]

Increased requirements (curl/lib/etc, unless we can find native simple lua HTTP post lib)

Recently I found a http lib based on neovim bundled libuv: https://github.com/sfenzke/http-client.nvim.git

that way you could run say :FzfLua live_grep instance=1 and FzfLua files instance=2 and then you have a persistent live grep/files

I found there're still many limitations even if use HTTP API, since many flags are one-way-door (behavior like --delimiter/--header-lines cannot be changed later by curl a fzf action). So to ensure a picker work as expected we cannot choose instance=n randomly...

We have to drop skim

Forgot about this. Just open a issue to track: skim-rs/skim#719

FzfLua has been very stable for a while now, this will be a lot of work/testing all while being in the “nice to have” category (not really user driven because we’re “missing” something)

Yes, I'm comfortable with current fzf-lua's performance/rich-feature.
Need a proper motivation, since instance=n is not trivial to have even currently in a http way, the other benefit I can think of is that we can handle all keymap in neovim (rather than by --bind). But that may not worth a major refactoring currently.

[ibhagwan]

I found there're still many limitations even if use HTTP API, since many flags are one-way-door (behavior like --delimiter/--header-lines cannot be changed later by curl a fzf action). So to ensure a picker work as expected we cannot choose instance=n randomly...

I feared the HTTP API isn’t there yet to accommodate all the features we need.

the other benefit I can think of is that we can handle all keymap in neovim (rather than by --bind). But that may not worth a major refactoring currently.

Combining actions/keymap.fzf/keymap.builtin into one table with neovim style binds is also something that was on my mind for a while, it’s even possible with the current architecture just a tiny bit more complicated to design, but then again there needs to be proper motivation as the onky benefit for this is nicer/simpler configuration.

[phanen]

@ibhagwan I checkout your SIGWINCH commit again know why it didn't work:
it send signal to /usr/bin/sh rather than its fzf child.

Patch on your commit that work:

diff --git a/lua/fzf-lua/profiles/hide.lua b/lua/fzf-lua/profiles/hide.lua
index acb7f9c..4187370 100644
--- a/lua/fzf-lua/profiles/hide.lua
+++ b/lua/fzf-lua/profiles/hide.lua
@@ -82,10 +82,11 @@ return {
         end
         return act
       end, opts.actions)
-      -- Hijack the resize event to reload buffer/tab list on unhide
-      opts.keymap.fzf.resize = function()
-        print("resize called", opts._oneshot_fzf)
-      end
+      opts.actions.resize = { fn = fzf.actions.dummy_abort, reload = true }
+      -- -- Hijack the resize event to reload buffer/tab list on unhide
+      -- opts.keymap.fzf.resize = function()
+      --   print("resize called", opts._oneshot_fzf)
+      -- end
       return opts
     end,
   },
diff --git a/lua/fzf-lua/win.lua b/lua/fzf-lua/win.lua
index 0c1828d..7a1d285 100644
--- a/lua/fzf-lua/win.lua
+++ b/lua/fzf-lua/win.lua
@@ -1217,7 +1217,8 @@ function FzfWin.unhide()
   -- Send SIGWINCH to to trigger resize in the fzf process
   -- We will use the trigger to reload necessary buffer lists
   local pid = fn.jobpid(vim.bo[self._hidden_fzf_bufnr].channel)
-  libuv.process_kill(pid, 28)
+  -- libuv.process_kill(pid, 28)
+  vim.tbl_map(function(_pid) libuv.process_kill(_pid, 28) end, vim._os_proc_children(pid))
   vim.bo[self._hidden_fzf_bufnr].bufhidden = "wipe"
   self.fzf_bufnr = self._hidden_fzf_bufnr
   self._hidden_fzf_bufnr = nil

[ibhagwan]

@ibhagwan I checkout your SIGWINCH commit again know why it didn't work:
it send signal to /usr/bin/sh rather than its fzf child.
Patch on your commit that work:

Nice @phanen! I still haven’t gotten to it as I’m busy with a new open source project I will soon(ish) publish :)

Do you find the SIGWINCH approach cleaner?

[phanen]

At least better than feeding termcodes...?

[ibhagwan]

At least better than feeding termcodes...?

I agree.

[phanen]

Related: #1901 (reply in thread).

[ibhagwan]

@phanen, will you look at my latest PR branch commit main...pr_1871 and let me know what you think?

Instead of unconditionally calling reload on every resize (fake or not) I modified the resize event to a transform that only calls the reload once upon our "fake" resize and only does so with pickers that have opts.__reload_cmd (buffers, tabs, others).

The only thing I'm not sure is if I want to add a var to enable the "hide-reload" only for specific pickers or do so for all pickers that have opts.__reload_cmd?

[phanen]

The only thing I'm not sure is if I want to add a var to enable the "hide-reload" only for specific pickers or do so for all pickers that have opts.__reload_cmd?

IMO that's enough since those buffer is not performance critical? I'm not those using thousand of buffers but I test Buffers/Tabs by argadd **/*.lua in neovim repo it work instantly when unhide.

But there seems a bug (racing?) after I open :Buffers then hit hide shortcut then very quickly hit unhide shortcut:

Error executing vim.schedule lua callback: /home/phan/b/fzf-lua/lua/fzf-lua/shell.lua|313| /home/phan/b/neovim/runtime/lua/vim/shared.lua:273: t: expected table, got nil
|| stack traceback:
|| [builtin#36]: at 0x560d3fc38020
lua/fzf-lua/shell.lua|313| in function 'fn'
lua/fzf-lua/shell.lua|77| in function </home/phan/b/fzf-lua/lua/fzf-lua/shell.lua:76>

Here: the second buflist passed in may be nil. No idea why but that's minor problem. No one really typing that fast to close then reopen.

fzf-lua/lua/fzf-lua/providers/buffers.lua

Line 187 in ee80318

local filtered, _, max_bufnr = filter_buffers(opts, core.CTX().buflist)

[ibhagwan]

Ty @phanen for the feedback, aeff813 merged into main.

But there seems a bug (racing?) after I open :Buffers then hit hide shortcut then very quickly hit unhide shortcut:

If this becomes a problem we’ll deal with it then.