glacambre / firenvim

Embed Neovim in Chrome, Firefox & others.
GNU General Public License v3.0
4.57k stars 144 forks source link

Allow for custom write function. #1589

Open ces42 opened 4 months ago

ces42 commented 4 months ago

This patch defines a (vimscript) function FirenvimWrite that just gets the current buffer content, but can be overriden by the user. This allows the user to do custom pre-processing of the buffer before it is written into the browser's text field.

This is a rather nice use case, but at least for me it is very useful. I use it on math stackexchange to make Firenvim expand my favorite latex macros.

justinmk commented 4 months ago

are the package-lock.json changes intentional

ces42 commented 4 months ago

are the package-lock.json changes intentional

No, sorry.

glacambre commented 4 months ago

Hi, thanks a lot for taking the time to improve Firenvim! I think this feature is very cool, and I believe I've had people asking for something like that before (mostly to pre/post-process the HTML of the elements they were editing).

I wonder if we couldn't make this more Vim-idiomatic though, by making Firenvim use the BufWriteCmd autocommand instead. This would let users define BufWritePre and BufWritePost autocommands that would modify the buffer as they expect, e.g.:

function FirenvimBufWriteCmd(chan, filename)
    call execute("doau BufWritePre " .. a:filename)
    call writefile(nvim_buf_get_lines(0, 0, -1, 0), a:filename)
    call rpcnotify(a:chan, 'firenvim_bufwrite', {'text': nvim_buf_get_lines(0, 0, -1, 0), 'cursor': nvim_win_get_cursor(0)})
    call execute("doau BufWritePost " .. a:filename)
    set &modified=0
endfunction
autocmd BufWriteCmd ${filename} FirenvimBufWriteCmd(${chan}, '${filename}')

And then the pre/post-processing is just:

autocmd BufWritePre secret.txt norm ggg?G
autocmd BufWritePost secret.txt norm ggg?G

What do you think?

ces42 commented 4 months ago

I tried doing something like that. I ran into two issues though. First, the changes from the autocmds start polluting the undo list. Second, processing the buffer twice per change might incur a rather heavy performance penalty for large buffers. Both of these problems are quite major when you configure firenvim to autosave, possibly several times per second, which I think is quite common.

glacambre commented 4 months ago

I feel like both of these concerns could be solved by having the pre/post-processing functions being smarter, e.g.

au! BufWritePre secret.txt let lines = nvim_buf_get_lines(0, 0, -1, 0) | undojoin | norm ggg?G
au! BufWritePost secret.txt undojoin | call nvim_buf_set_lines(0, 0, -1, 0, lines)

But once we reach this point, it's true that we're getting farther and farther away from "just regular vim scripting". Moreover, this wouldn't be robust against multiple BufWritePre autocommands running at once (e.g. one that prettifies the buffer and another one that expands some patterns).

We should probably also take into account the dimension of re-use. It's unlikely that the pre/post-processing required in Firenvim will be needed outside of Firenvim, so from this point of view, a function specific to Firenvim's API would be fine. However, it's likely that any BufWritePre/BufWritePost autocommand created by users outside of Firenvim does need to apply inside of Firenvim (e.g. users who have pretty-printing of their JS code while working in their terminal also want it when editing JavaScript on the CodePen website), so from this point of view, the BufWriteCmd approach would have an advantage.

I'll need to think some more about this. I guess it'll come down to the likelihood of each situation happening, which I have a hard time reasoning about.

glacambre commented 2 months ago

@ces42 I apologize for forgetting about your PR, I haven't spent much time on Firenvim these days. I had to publish an urgent release to fix a bug that completely broke Firenvim with Neovim nightly, I took advantage of this to sneak in a commit that might do what you want.

It's a bit more of a hassle to use than what you suggested in your PR, but I went with my approach because it followed a pattern that already exists with other neovim<->browser requests and I did not have time to think more about the best solution.

Basically, you can now call a function named firenvim#write(), that takes two optional parameters (a list of strings and a tuple of numbers) and tells the browser to write the list of strings to the textarea Firenvim is attached to and move the cursor to the position specified by the two numbers of the tuple.

So instead of redefining a function named FirenvimWrite like in your PR, users would manually call firenvim#write() when they want to. It's not as nice/usable so I won't close your PR, but I just wanted to let you know that there now was a workaround.