Add an option to remove '\n' from `uv.fs_write` calls

[ibhagwan]

https://github.com/vijaymarupudi/nvim-fzf/blob/84e119698ad6adfb787437310918339727092827/lua/fzf.lua#L194

The above is what I’m talking about, I’ll explain the use case below.

For example, fzf-lua grep (and live_grep) runs a command using uv.spawn, due to \n being added by raw_fzf, I cannot send the raw command output (already containing \n) directly to the fzf pipe (using fzf_cb calls), instead the data must be parsed and sent line by line (after also stripping the \n), needless to say parsing the lines is not always very performant, especially when dealing with large datasets, not having to parse the newlines results in a major performance improvement (this also avoids the “out of memory error” that was described in fzf-lua#185)

Granted in such case one could simply send the command as string in contents but that isn’t always possible in my code for various reasons (also adds “conditional” complexity instead of always using uv.spawn).

Here is what I do in my code, if the output doesn’t require line by line transformation (I.e. no icons are needed for files), I simply pass the data directly to the pipe without parsing. Given that the data isn’t guaranteed to end in a new line this doesn’t work well with raw_fzf, effectively forcing newline parsing:

https://github.com/ibhagwan/fzf-lua/blob/b87e5da1c25abf5fa3af15b750fafb85dca433c5/lua/fzf-lua/libuv.lua#L131-L132

[vijaymarupudi]

I think this feature is long overdue! I'm planning a major refactor of the fzf.lua internals, and will provide raw_fzf_pipe, which will expose the vim.loop pipe directly, which I think will solve this issue!

[vijaymarupudi]

Okay, could you try the core-refactor branch and make sure everything is functional? The callback in the fzf function will now also provide you the vim.loop pipe as the second argument, allowing you to use it as you see fit!

[ibhagwan]

Okay, could you try the core-refactor branch and make sure everything is functional? The callback in the fzf function will now also provide you the vim.loop pipe as the second argument, allowing you to use it as you see fit!

Didn't really do a through test yet but it seems to work, to properly test it I have to now add \n where appropriate in all the use cases where contents is a function and either add \n or remove the code that strips the \n.

I'm using my own implementation of the libuv calls now (better code reuse on my end) so it wouldn't be an issue for me but when you do merge this do not forget to remove the \n stripping from cmd_line_transformer.

[vijaymarupudi]

Just to clarify, the newline is still being added by default if you use the function that passed to callback i.e. all code should be backwards compatible. I assume you did not have to make any changes to make it work?

And yes, I will make sure I remove the newline parsing

[ibhagwan]

It's not backwards compatible in the sense that the below code works before the change and stops working after the change, before the change each callback resulted in one line and now without the \n it won't separate the lines:

function M.test()
  coroutine.wrap(function ()
    vim.cmd("new")
    require('fzf').raw_fzf(function(fzf_cb)
        for i=1,10 do
          fzf_cb(i)
        end
        fzf_cb(nil)
      end,
      nil,
      { fzf_binary = 'fzf' })
  end)()
end

After the change this results in:

This happens because the line below previously added \n (not anymore), personally this is what I'm after so it's not a problem for me but it will require adjustments by anyone using contents as a function:

https://github.com/vijaymarupudi/nvim-fzf/blob/4716eec8b01ade4992ead5ec477e0f98940e1693/lua/fzf.lua#L218

[vijaymarupudi]

Okay, whoops, there is a commit that I didn't push yet, will push later today. There will be a newline, I just missed that. People who don't need newlines should use uv.write on the second argument to the callback, which will be the uv pipe.

[ibhagwan]

After playing with this for a while I found it was simpler (i.e. requiring the least amount of code changes) to add another variable to fzf_cb to skip adding "\n" when it is not required, for my purposes it was better than exposing the pipe directly and having to make changes across many files.

Having my own raw_fzf also gave me the option to not have to expose the write callback "counter logic" we discussed in #41 and basically have an "opaque" fzf_cb interface that I can call and not worry about anything else, the data is guaranteed to be written or err (if process was killed) and sending fzf_cb(nil) marks the end of data but will not close the pipe until all write callbacks have finished.

This works very well and is also showing better performance when I do not need to parse the lines (when icons aren't requested) as I can simply passthrough the data from uv.spawn directly to the output pipe.

Based on our discussion I understand why you'd want to keep nvim-fzf's interface more generic and not add "flag hacks" but just in case you're interested in applying some of this the code logic you can find it below: https://github.com/ibhagwan/fzf-lua/blob/main/lua/fzf-lua/fzf.lua

Together with the above and your fork (which I assume will soon be merged as well), this issue can be safely closed.

[vijaymarupudi]

Reopening the issue to keep track of this feature. This is what I'm planning. First argument is the function that adds newline, second arguments will be a similar function (the API you preferred) that doesn't add a newline. The third argument will be the raw libuv pipe, for people who want maximal control. I assume this solves all issues?

[ibhagwan]

Reopening the issue to keep track of this feature. This is what I'm planning. First argument is the function that adds newline, second arguments will be a similar function (the API you preferred) that doesn't add a newline. The third argument will be the raw libuv pipe, for people who want maximal control. I assume this solves all issues?

Sounds like it would cover all bases but don’t feel like you have to do this just for fzf-lua, this is quite an advanced use of your plugin and I wouldn’t be surprised if only fzf-lua is using it this way.

[vijaymarupudi]

Oh no, I want to remove all constraints! Just a heads up, I will also be adding a write queue, I doubt anybody will notice anything changed, I will consider reverting if people complain (very unlikely). I think the usability benefit is too good (as you have mentioned in the past)

[ibhagwan]

Great, tysm!

[vijaymarupudi]

https://github.com/vijaymarupudi/nvim-fzf/tree/core-refactor

This pretty much amounts to a rewrite of core of nvim-fzf, please test everything that you can! Write queuing is supported, in addition to options to not send the new line, and providing the raw pipe in case people need it.

[ibhagwan]

https://github.com/vijaymarupudi/nvim-fzf/tree/core-refactor

This pretty much amounts to a rewrite of core of nvim-fzf, please test everything that you can! Write queuing is supported, in addition to options to not send the new line, and providing the raw pipe in case people need it.

Works great!

I like your WriteQueue implementation I think it can also be used in cmd_line_transformer and choices_to_shell_cmd_previewer (should also close #35).

Worth noting I haven't done the whole race condition testing with big files and job cancellations/process kills.

[vijaymarupudi]

Great! I doubt that the code would have changed for cancellations of large jobs, so after I use the WriteQueue implementation for the helpers, I think I will merge.

This patch also has windows support :)

[vijaymarupudi]

Should I merge now, it looks you have compat patches in fzf-lua?

[ibhagwan]

Should I merge now, it looks you have compat patches in fzf-lua?

I’m still weary of “big changes” because I always am when it comes to code but it’s most likely safe to merge :)

Ty!

[vijaymarupudi]

I'll keep an eye out on my email today, just in case we need a quick revert