Integrate with FastFold
kopischke opened this issue · 37 comments
FastFold by @Konfekt leverages the same events as vim-stay:
Regarding compatability with FastFold, vim-stay must be loaded after FastFold so that mkview in vim-stay's BufWinLeave and BufWinEnter autocmds saves the syntax or expression foldmethod and not the manual foldmethod FastFold artificially imposes.
The BufLeave autocmd is curious as it seems accounted for by the BufWinLeave autocmd ?! If vim-stay only used that autocmd, than the timing would always be wrong, because the BufLeave autocmd is invoked too early. However, as things stand, the erroneous (that is, incompatible to FastFold) view file created by the BufLeave autocmd is overriden by the correct one from the BufWinLeave autocmd.
The problem is that when quitting a window, BufWinLeave sometimes fires after the window has been destroyed (the window number returned by getbufwinnr(bufnr) is -1 – see :h BufWinLeave, which warns of this, albeit a bit cryptically), which makes view creation impossible (:mkview needs a valid window). BufLeave OTOH always fires in a valid window, so both events are needed to catch “buffer leaving a view” situations. This also means vim-stay will at times save the spurious manual folds set by FastFold.
I’m not sure I can fully solve that on my end. One thing I can think of which could work, provided me and @Konfekt both change a few things:
- refactor vim-stay to trigger view creation on a User autocommand (say
BufViewLeave), with the current autocommands triggering this event instead of saving the view directly – that would be my end; - have FastFold do
silent doautocmd User BufViewLeaveafter it restores the fold method onBufWinLeave– that would be @Konfekt’s end.
Pros: load order of the plug-ins doesn’t matter anymore and all corner cases should be covered.
Cons: views will be saved twice when vim-stay’s BufWinLeave handler is triggered after FastFold’s.
EDIT: the alternative would be to integrate on my end only by checking for FastFold’s w:lastfdmand b:lastfdm. @Konfekt: some help about how these interact in practice would be appreciated.
Ok, sounds like a fine suggestion. As the only con is what's already status quo. So things can only get better.
So pasting silent doautocmd User BufViewLeave after s:Leave() | at
https://github.com/Konfekt/FastFold/blob/master/plugin/fastfold.vim#L214 and there we go?
Feel free to provide a pull request if it's more intricate than that, most easily discussed that way.
I’ve got something slightly more intricate in the works. Branch should be ready in half an hour at most, will let you know.
OK, I’ve rustled up something like an extensible, generic extension API for vim-stay, with FastFold integration as the first (and right now only) implementation. Care to take a look at (and maybe play around with) the fastfold-integration branch? No docs yet, I’ll add that if the scheme pans out.
Ok, looks fine. And as if no change on my end is any longer needed?
The only worry is that it depends on FastFold variables actually hidden to the user, so hopefully they will never change.
Perhaps I should document them as a reminder.
If you can see your way to APIfying your variables (i.e. documenting them and guaranteeing they will be present, named and functioning like they are currently in the future, barring major version changes in FastFold), yup, you can safely leave integration to me. Or you could pull in autoload/stay/integrate/fastfold.vim (with that exact directory path) into FastFold, to the same effect – I can prepare a PR for that if you want – as the extension system works wherever in rtp the integration script is stored. The latter option would allow you to change the inner workings of FastFold without worrying about breaking compatibility with a 3rd party plug-in, but it will add 1 Kb of code to your own.
Either way is fine with me, just let me know :).
Alright, as FastFold is a dead-simple plugin, I opted for option 1: You find
and its improvement
that document the w:lastfdm variable. The b:lastfdm variable is a crutch for guessing w:lastfdm.
Cool, I’ll merge the branch tomorrow and make FastFold support official. One last question: should I ignore b:lastfdm and only query the window-local variable?
Yeah, I saw that line and therefore commented on it. Can be safely ignored: By the time FastFold changes the foldmethod, always the window local version is used. The b:lastfdm stores the w:lastfdm of the same buffer that may be displayed in another window or tab; it is only one of many sources for guessing w:lastfdm.
I have a problem with this not working when I have multiple buffers/windows open. If I put some diagnostics in the save_pre and save_post functions I can see that when I run 'zuz' in FastFold the pre and post functions are being called twice on each file (I assume due to the windo Enter and Leave). I'm not sure if this is correct behaviour just on the 'zuz', but the problem appears to be in the post condition which only sets the fdm to manual if it differs from the FastFold saved value.
I think this behaviour should instead be to set fdm to manual if the FastFold saved value (w:lastfdm) exists. This change has fixed it for me, but I don't understand the autocommand interactions well enough to know if there are side effects, or if this is the correct solution.
Hmm, that’s odd, as the two BufStaySave\* events are only separated by the session save command. Could you run the following for me, please?
set verbosefile=/somewhere/you/will/find/it/log.txt
augroup staydebug
autocmd user BufStaySavePre,BufStaySavePost verbose echo reltimestr(reltime()) expand('<amatch>') expand('<abuf>') bufnr('%') winnr() &fdm get(b:, 'lastfdm', '')
augroup ENDand try to trigger the error you described? Once you have done so succesfully, do
set verbosefile=
autocmd! staydebug
augroup! staydebugor restart your Vim session and paste the contents of the above log.txt file here (unless it is humungous, then I’d prefer you upload it somewhere and just link to it in a comment :))
Hmm..., perhaps the issue is that FastFold only sets fdm=manual if the fdm was syntax or expr. Other foldmethods are computationally little demanding, and so there is no need to artifically impose fdm=manual.
I won't be able to run the debug for another 12 hours, but I think the issue is that the 'zuz' command cycles through all the windows but only changes the fdm on those that match the current buffer (this behaviour is correct). By contrast, the save_pre always restores the original mode before saving (which is also correct) but because FoldFast is ignoring some windows the fdm is not restored by FoldFast and so is not different in the save_post and so is not restored by that either.
I believe the save_post behaviour as it stands is either redundant (when FoldFast restores the fdm itself) or non functional (when FoldFast does not restore the fdm) depending on the circumstances.
Hmm, that seems unlikely. to me: as I said, the only thing happening between the BufStaySavePre and BufStaySavePost events is the view session creation via :mkview (that is literally the whole code that is there), and barring exceptions aborting execution, the events will always fire in tandem.
@Parakleta I’m afraid I really do need that debug output – take your time, 12 hours hence for you means I won’t look at it before next morning my time (I’m on GMT+1) :). Also, could you specify what exactly “is not working” ?
What @Parakleta says seems to nail it, see https://github.com/kopischke/vim-stay/blob/master/autoload/stay/integrate/fastfold.vim#L15 and https://github.com/kopischke/vim-stay/blob/master/autoload/stay/integrate/fastfold.vim#L23.
Supposedly that BufSave command is doing harm because FastFold cycles through all windows of the current buffer, and also only sets the foldmethod to manual if they were syntax or expr before.
Best solution: Store l:fdmlocal in w:fdmlocal https://github.com/kopischke/vim-stay/blob/master/autoload/stay/integrate/fastfold.vim#L15 and replace 'manual' in https://github.com/kopischke/vim-stay/blob/master/autoload/stay/integrate/fastfold.vim#L23 by w:fdmlocal.
Hah, I think the issue is much simpler: I stupidly set the condition for restoring FastFold’s enforced manual fold method to always evaluate to false when the folds have been reset to w:lastfdm in BufStaySavePre. Replacing Lines 21 and 22 of stay#integrate#fastfold#save_post() by this
if &foldmethod isnot# 'manual' && exists('w:lastfdm') should fix the issue. @Parakleta give it a try and report back, please?
That works for me. It's basically the same as what I figured out. Should the &foldmethod be &l:foldmethod instead?
I still have an issue with FastFold not setting the mode to manual when first loading a buffer but I think this is unrelated, so I'll keep digging for that.
ETA: Nevermind, I was loading the plugins in the wrong order. I wonder if there is a way to make it not order-dependent?
It's basically the same as what I figured out.
It’s exactly what you figured out originally, actually :). The whole ensuing thread about duplicated events, cycling windows and FastFold restoring stuff just obfuscated that for me. Sorry for being slow, and for introducing the bug, of course – will fix in next release.
Should the &foldmethod be &l:foldmethod instead?
&setting gets the actual setting; local if set locally, global if not, while &l:setting gets you nothing when the setting is global, so the former is the correct test here (bit of a moot point in our case, really, as foldmethod is window-local, but we cover possible future shifts in its semantics that way).
I wonder if there is a way to make it not order-dependent?
The integration module’s point is to do exactly that: ensure vim-stay and FastFold play nice with each other, independently of load order. I think we (@Konfekt and I) might have overlooked the load scenario when we talked about compatibility. @Parakleta try what happens when you change this line of the patched integration module to:
autocmd User BufStayLoadPost,BufStaySavePost unsilent call stay#integrate#fastfold#save_post()That change has the effect of resetting the fdm to the default setting rather than the view setting. FastFold's lastfdm is set to the default value that prevailed before the view was loaded (by its BufWinEnter firing before loadview) and then save_post is restoring that value rather than the post-view value.
I think you could doautocmd FastFold BufWinEnter if g:loaded_fastfold after loading the view but it feels a bit clumsy. It's starting to feel too coupled between the two plugins.
FastFold could export its BufWinEnter and BufWinLeave actions as functions and you could stitch them into your pre/post load and save behaviour and disable the equivalent FastFold autocommands, effectively hijacking its behaviour and sequencing it yourself.
As a general integration architecture I think something like that may be your only option since you have no control over the order of BufWinEnter and BufWinLeave autocommands. In fact a specific plugin load order wont work in general because you want the Enter and Leave sequences to be in opposite orders as a stack.
That change has the effect of resetting the fdm to the default setting rather than the view setting.
It should set it to manual; I thought the problem was
FastFold not setting the mode to manual when first loading a buffer
and that should fix it as far as interference from loading a vim-stay view is concerned, provided you have patched save_post() as described above.
Sorry, I wasn't very clear in my description. I skipped a bit.
The change you have proposed does leave the l:foldmethod set to manual correctly, the issue is that FastFold never gets to see the loadview fdm value, and so its lastfdm value holds the default value. Basically save_post() is wiping out the loadview fdm value before FastFold has a chance to cache it.
Ah, now I understand – you’re right of course, the view session can load a different foldmethod. That seems easily solvable, tho, by calling a dedicated load_post() instead of save_post() in the BufStayLoadPost autocommand, which would issue a FastFoldUpdate command.
Yes of course, FastFoldUpdate does exactly what is needed here. I'm about to sign off for the weekend, I'll have another fiddle and do some more testing on Monday.
@Parakleta I’ll do the fiddling, you do the testing :). Check this thread on Monday.
@Konfekt any furthers thoughts?
I am a bit lost. Regarding
That change has the effect of resetting the fdm to the default setting rather than the view setting. FastFold's lastfdm is set to the default value that prevailed before the view was loaded (by its BufWinEnter firing before loadview) and then save_post is restoring that value rather than the post-view value.
one should note that there is also a SessionPost autocmd event fired by every loadview (there is do autocmd SessionPost in every view file) that resets fdm to manual.
@Parakleta, what exactly goes wrong with the proposed solution at #5 (comment) ?
one should note that there is also a SessionPost autocmd event fired by every loadview (there is do autocmd SessionPost in every view file) that resets fdm to manual.
Good call – the root cause of the load issue is that vim-stay suppresses all autocommands when loading a session as I found it significantly slows down buffer window entering, even on a fast machine and when there are no autocommands listening to SessionLoadPost. That is fine for the occasional manual load, but it’s a deal breaker for a plug-in that does load sessions as often as vim-stay.
I have added manual triggering of SessionLoadPost to the view restoration of the upcoming release, which doesn’t seem to cause the same slowdown (not sure I even want to know… the Ways of Vim are Inscrutable) – that should fix that part of the interaction with FastFold.
@Parakleta could you give the preliminary 1.2 branch a whirl?
I cannot make it misbehave now, seems all good. Thanks.
Hmm.. Works fine when thr user uses syntax or expression folding, but in fact FastFold does not enter at all for other foldmethods. So resetting the fold method to thr previous one on the Post event is more compliant..
resetting the fold method to thr previous one on the Post event is more compliant..
Not sure if I follow your meaning. Are we talking about BufStaySavePost here?
Yep, but I was mistaken. In that case the lastfdm is not Set. So I think it's Alright.
In that case the lastfdm is not Set.
vim-stay never touches the fold method unless w:lastfdm is set, so I guess this is not an issue solvable on my end (if it is one at all?). Will release tomorrow, then.
Yep, looks good. I think FastFold never creates lastfdm if it doesn't touch anything.
Good to know, thanks :), For the purpose of this integration module, I am going to assume that the state of w:lastfdm is consistently well defined (i.e. it is always set to the last user defined fold method when FastFold is active, and unset when it is not). That is its API contract, right?
I pushed a commit that ensures this. Hopefully this does not break anything else.
Is all of this going to be reflected in your pull request as well? I will think about it when I've more leisure.
Is all of this going to be reflected in your pull request as well? I will think about it when I've more leisure.
I’ll update it to reflect the changes, then.