PR #45 (parinfer on TextChanged) is awesome, but really messes up undo history!

[jschomay]

I'm delighted to get the new functionality from #45, but it seems to create a ton of changes in the change list, messing up my undo history.

To replicate:
Make a change somewhere. Then do something that will trigger this new feature (I typed "()" before some other code, pressed 'esc', then delete the second ")" with 'x', which inserted a new ")" near the end of the form, as expected).

Now try using undo.

For me, first the parinfer edit gets undone, but then on the next undo, the cursor jumps to the beginning of the root form and it says it is doing undos, but nothing changes, and I can hit undo for a long time without undoing the change I made before triggering the parinfer TextChanged behavior.

I hope that explanation is clear, and I hope my blame is accurate. I've only noticed this after the update.

As it is, I am unable to get back to a previous state, which is very hampering, and I've had to disable the plugin for now :(. I am hoping there is a way to do the new behavior without adding to the change list, which I think should solve the problem.

Thank you!

[shaunlebron]

we've had to resolve this problem in atom-parinfer with an option to skip the undo stack when updating the text buffer: https://github.com/oakmac/atom-parinfer/blob/v1.22.0/src-cljs/atom_parinfer/core.cljs#L483

something like this probably exists for vim

[bhurlow]

ahhh yes, I knew something mysterious had changed. This makes sense. Seems like some option to bypass the undo history is what we need, this is outside my immediate vim knowledge but is likely supported

[jschomay]

I've tried to dig in a little bit more. I've never done vimscript, so this is shooting in the dark, but here are things I found that seem helpful:

1. (possibly a red herring) - vim docs for TextChanged say "Careful: This is triggered very often, don't do anything that the user does not expect or that is slow." I don't think this is the problem though, see my next point.

2. Looking at :changes, everything looks right to me up until the point I hit undo. Instead of removing items from the change list, it seems to add changes! The change location reference the line after the current root form, and the beginning line of the root form, and in addition, when pressing 'u', the status line says "n changes...", where n is the number of lines in the current root form. This leads me to believe that the problem lies in parinfer#draw(), specifically the for loop calling setline(). And/or possibly parinfer#process_form(), specifically the call to setpos(). Perhaps the most interesting insight is that this behavior only seems to happen when trigger from TextChanged, but not the other triggers, like InsertLeave.

Hopefully this is helpful!

[jschomay]

Thinking more about this, I think the problem is that TextChanged triggers when an undo happens, which calls process_form, which creates more changes. So the solution is either to avoid 'undo' triggering anything, or to avoid creating changes from any of the parinfer functions. The latter is preferable, as opening a new clojure file should start with an empty change list, but that is not the case.

Still not sure how to do that though...

[bhurlow]

@jschomay thanks for digging! Ideally we could so something like ~~ TextChanged() && !undoAction

[bhurlow]

I'm interested by the thought of storing a separate set of changes related to CLJ editing, i.e. the undo could restore at the structural level rather than a single paren. This may be a separate project idea though

[jschomay]

Last update - it seems undojoin is the answer here, but I haven't been able to get it to work. Here are two use cases, but repeating these hasn't worked for me (because it won't fire after an undo, even though these links seem to indicate it does). Hopefully someone else will figure it out.

• https://github.com/sbdchd/neoformat#managing-undo-history
• https://github.com/clojure-vim/nvim-parinfer.js/blob/master/plugin/parinfer.vim#L60

[jschomay]

Ok, one more thought, I've also seen other plugins that do things like formatting, where they write to a separate buffer, then dump that all in the current one, which might help. At least I think they do that, like I said, I'm not a vimscript developer.

This might be very helpful to look at / copy. You can see how he is using a new buffer to do the formatting, then swapping it back, using undojoin and also something with an undo file: https://github.com/ElmCast/elm-vim/blob/master/autoload/elm.vim#L56

[spinningarrow]

Thanks for reporting this! Would you be able to test with the latest changes in #46? I think my last commit there goes some way in fixing this (it seemed to have fixed it locally for me).

[jschomay]

@spinningarrow I tried with your changes, and yes, it seems to fix the problem! Nice one, thank you! I haven't used it beyond my simple test, so I'm not sure if there are other unintended effects, but it seems good, and certainly is more usable than before.

[spinningarrow]

Just ran into this again, looks like we need a better fix 😞

[bhurlow]

@spinningarrow noted 😢

[spinningarrow]

This has been reported as an issue in Vim: vim/vim#3291 (thanks to @eraserhd for the digging!)

EDIT: fixed issue link

[spinningarrow]

The vim issue above has been fixed in patch 245. I tried the latest vim on macOS (using brew install vim --HEAD) which includes that patch and saw that while it does help with the issue of messed up undo history, it doesn't completely fix it.

I've made another PR - #53 to attempt to fix it; would love it if you all could test it and provide your feedback! It still has issues I think, but it prevents the main "undo loop" issue.

[eraserhd]

Bram later added patch 8.1.0256, which should Just Work(tm) -- it does for parinfer-rust, for sure (although I just precede setline() with silent! undojoin).