allow the user to set different fzf binary

[ibhagwan]

This allows to set a custom fzf_binary, I did this mostly to be able test skim instead of fzf, works great, pretty much all options are compatible aside from the change:reload which skim implements with -i (interactive) mode.

[ibhagwan]

fzf requires another command or stream as input, skim has an interactive mode which is similar to the change:reload event in fzf where it reloads the command based on user input, this small patch enables the user to send contents=nil to raw_fzf and subsequently ignoring the input stream and running the command directly.

Feel free to drop this patch and do this in any way that fits your code and coding style better :-)

[vijaymarupudi]

That's interesting, yes, I want to support this use case. For specifying the binary, I think I would prefer it go through the process_options mechanisms, so that it can be both individually, or globally specified (making skim an officially supported binary). The name of this library... it is what it is.

Do you want to take a shot at that first? Otherwise I can get to it on Sunday (life's pretty busy right now).

For the second commit, It's not entirely clear to me why you check for the length of the string, if contents is going to be nil? Otherwise looks pretty good, need to add documentation though, but I can do that.

[ibhagwan]

That's interesting, yes, I want to support this use case. For specifying the binary, I think I would prefer it go through the process_options mechanisms, so that it can be both individually, or globally specified (making skim an officially supported binary). The name of this library... it is what it is. Do you want to take a shot at that first? Otherwise I can get to it on Sunday (life's pretty busy right now).

process_options would be the right home for this but when looking at your code that would require an additional argument to (some) function(s) (i.e. process_options(fzf_binary, fzf_cli_args, window_options) which I wasn't a fan of, I prefer a table of arguments which is cleaner but that would be a bigger mess to your code which I didn't know if you'd like :-)

"Hacking" the global FZF seemed like an easy compromise but I totally understand. Let me know what you think?

For the second commit, It's not entirely clear to me why you check for the length of the string, if contents is going to be nil? Otherwise looks pretty good, need to add documentation though, but I can do that.

As part of my testing trying to avoid the input stream I sent contents='' as a zero-length string which resulted in a bug as it's trying to pipe nothing, I thought I'd fix this along the way.

[ibhagwan]

One more thing, the documentation specifies to call cb(nil) (in multiple locations in the README) to signal end of processing which ends the fzf loading indicator, that fails with an exception as the code expects the user to send a callback function to call after running on_done() at line 114 of fzf.lua.

That may not be an issue with fzf which is very robust and not calling this on async operations (LSP functions, etc) wasn't causing any issues but with skim the loading indicator never stops and the process goes to 100%.

I fixed that by calling cb(nil, function() end) to signal I was done with processing, it might be worth it to fix the documentation or better yet change L#114 to if cb then cb(nil) end instead?

[vijaymarupudi]

The string error checking sounds good!

I would go ahead and change the option handling to tables if you find the time. I'm agnostic to style in this case, since this library is small enough for the mental overload to not be too high. Whatever you think is best to understand the code!

Additionally, if you'd like, I would also make the change to line 144, that sounds good, since that operation can never fail.

[ibhagwan]

Not extremley happy with the solution, it would have been much easier to just refactor the whole options part, set one defaults_options structure and then vim.tbl_deep_extend it with the user options, unfortunately I was limited by backward compatibility. I wouldn't want to introduce breaking changes to the existing user base.

What I'm mostly unhappy about is the fact that we call process_options twice when calling FZF.fzf, not really a problem but just feels bad from the architecture standpoint.

[ibhagwan]

I made an additional tiny change to potentially solve fzf-lua issue#20, take a look and let me know what you think?

Based on the video the @sQVe sent it seems that the FZF.fzf function successfully returns (otherwise the selected file wouldn't be opened for edit) but the call to :bw! .. doesn't kill the window properly, it also seems from the video that the term process doesn't end properly but I'm not sure?

In any event I think it's wiser to use vim.api.nvim_buf_delete over vim.cmd("bw!...") (which I see you already used in FZF.provided_win_fzf).

[ibhagwan]

Read the issue and solution in fzf-lua issue#20, really not sure what in glepnir/dashboard-nvim causes this behavior but adding the wincmd q seems to solve it.

[ibhagwan]

Don't merge the last commit yet, this causes an issue when the sending buffer is unlisted, say I open fzf from a help or man page, and then open fzf again the wincmd q would execute on the sending page and quit vim, I tried verifying the current win id before sending wincmd q and it still closes the unlisted buffer.

I took a different approach I send the wincmd q later only if the winid is still valid.

[ibhagwan]

What about the fix for issue #20 and the conditional wincmd q addition? it seems to have fixed the conflict with dashboard but I’m still not sure what code in that plug-in causes this behavior.

[vijaymarupudi]

Huh, my comment for that seems to have gotten lost. I was curious when you used wincmd instead of nvim_win_close?

[ibhagwan]

Huh, my comment for that seems to have gotten lost. I was curious when you used wincmd instead of nvim_win_close?

I tried that as part of my original attempt to fix this issue but for some reason it didn’t work and only wincmd q closed it, perhaps since “take #2” of the fix it’s now conditional and I also switch to the window before with is_valid and set_ current_winso it might work, if so I’d also prefer nvim_win_close.

This can quickly be verified by installing glepnir/dashboard (I can test a bit later when I’m on the computer).

[ibhagwan]

Alright, just tested, nvim_win_close didn't solve it but vim.api.nvim_win_close(winid, {force=true}) did.

[ibhagwan]

Just applied another potential fix for compatibility with the dashboard-nvim plugin, changed the order of closing first the window and then wiping the buffer, not sure why this was causing an issue with dashboard but it makes more sense anyway.

[vijaymarupudi]

Just merged the pull request. There were some subtle bugs I had to fix, which involve reassigning the global options object, having no fzf_cli_args, etc.

I hope this is good! You can read the documentation for the updated behavior.

One note: Next time, please make separate pull requests for each fix/feature, makes it easier to me so that I can review and push it faster. Thank you very much for your contribution!

[ibhagwan]

Much appreciated, ty!