Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for custom write function. #1589

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Allow for custom write function. #1589

wants to merge 11 commits into from

Conversation

ces42
Copy link

@ces42 ces42 commented Feb 19, 2024

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
Copy link
Contributor

are the package-lock.json changes intentional

@ces42
Copy link
Author

ces42 commented Feb 20, 2024

are the package-lock.json changes intentional

No, sorry.

@glacambre
Copy link
Owner

glacambre commented Feb 20, 2024

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
Copy link
Author

ces42 commented Feb 20, 2024

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
Copy link
Owner

glacambre commented Feb 20, 2024

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 added a commit that referenced this pull request Apr 28, 2024
The goal of this commit is to enable the use case desired by #1589. This
isn't a perfect fit as the user was asking for a way to setup
post-processing of their text instead of having to call firenvim#write,
but as I am on a tight schedule for publishing a new release I do not
have time for further discussions, and following the pattern of
firenvim#hide_frame, firenvim#eval_js, firenvim#focus_* feels like a
very safe choice.
glacambre added a commit that referenced this pull request Apr 28, 2024
The goal of this commit is to enable the use case desired by #1589. This
isn't a perfect fit as the user was asking for a way to setup
post-processing of their text instead of having to call firenvim#write,
but as I am on a tight schedule for publishing a new release I do not
have time for further discussions, and following the pattern of
firenvim#hide_frame, firenvim#eval_js, firenvim#focus_* feels like a
very safe choice.
glacambre added a commit that referenced this pull request Apr 28, 2024
The goal of this commit is to enable the use case desired by #1589. This
isn't a perfect fit as the user was asking for a way to setup
post-processing of their text instead of having to call firenvim#write,
but as I am on a tight schedule for publishing a new release I do not
have time for further discussions, and following the pattern of
firenvim#hide_frame, firenvim#eval_js, firenvim#focus_* feels like a
very safe choice.
glacambre added a commit that referenced this pull request Apr 28, 2024
The goal of this commit is to enable the use case desired by #1589. This
isn't a perfect fit as the user was asking for a way to setup
post-processing of their text instead of having to call firenvim#write,
but as I am on a tight schedule for publishing a new release I do not
have time for further discussions, and following the pattern of
firenvim#hide_frame, firenvim#eval_js, firenvim#focus_* feels like a
very safe choice.
glacambre added a commit that referenced this pull request Apr 28, 2024
The goal of this commit is to enable the use case desired by #1589. This
isn't a perfect fit as the user was asking for a way to setup
post-processing of their text instead of having to call firenvim#write,
but as I am on a tight schedule for publishing a new release I do not
have time for further discussions, and following the pattern of
firenvim#hide_frame, firenvim#eval_js, firenvim#focus_* feels like a
very safe choice.
glacambre added a commit that referenced this pull request Apr 28, 2024
The goal of this commit is to enable the use case desired by #1589. This
isn't a perfect fit as the user was asking for a way to setup
post-processing of their text instead of having to call firenvim#write,
but as I am on a tight schedule for publishing a new release I do not
have time for further discussions, and following the pattern of
firenvim#hide_frame, firenvim#eval_js, firenvim#focus_* feels like a
very safe choice.
glacambre added a commit that referenced this pull request Apr 28, 2024
The goal of this commit is to enable the use case desired by #1589. This
isn't a perfect fit as the user was asking for a way to setup
post-processing of their text instead of having to call firenvim#write,
but as I am on a tight schedule for publishing a new release I do not
have time for further discussions, and following the pattern of
firenvim#hide_frame, firenvim#eval_js, firenvim#focus_* feels like a
very safe choice.
glacambre added a commit that referenced this pull request Apr 28, 2024
The goal of this commit is to enable the use case desired by #1589. This
isn't a perfect fit as the user was asking for a way to setup
post-processing of their text instead of having to call firenvim#write,
but as I am on a tight schedule for publishing a new release I do not
have time for further discussions, and following the pattern of
firenvim#hide_frame, firenvim#eval_js, firenvim#focus_* feels like a
very safe choice.
@glacambre
Copy link
Owner

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants