comparison to restore_view
Konfekt opened this issue · 29 comments
Hello Martin,
Thanks for the plugin.
Could you explain the errors you found in restore_view's heuristics which buffer views to save and restore?
For example, restore_view excludes all non-modifiable buffers wheras vim-stay seems to include them but explicitely disables netrw buffers.
It makes me wonder if non-modifiable buffers happen to be file buffers?
Hey, thanks for asking. I’ll try to explain:
- vim-stay’s heuristics do not exclude
nomodifiablebuffers because that is just a setting (see:h modifiable) that can indeed be applied to any file buffer. Even allowing for the assumption that it is never arbitrarily set, the fact a file is locked, read-only or otherwise non-modifiable does not imply the user does not care for its view state (IMO – help files are excluded viabuftypeas interfering with help tag jumps would get very confusing very fast). - the global volatile type list is just a last line of defence against legitimately “ordinary” file buffers that are volatile by their very nature (git commit messages and such) and those plug-ins failing to set any option that indicates their non-file nature. “netrw” is on it it because Netrw’s directory browser windows have no
buftypeset; by the same token, you will not find Unite’s, CtrlP’s, VimFiler’s, VimShell’s or Conque Shell’s file types there, as these plug-ins all set a non-filebuftype. - my criticism of restore_view’s buffer name based heuristics is a generic one: Vim has no concept of a file belonging to a buffer, just that of a buffer name. A buffer named “[Command line]” can be the command-line window, or a file named “[Command line]” in the current working directory; unless you look at
buftype, there is absolutely no way to tell (no, not even by looking for a matching file; it might be a new buffer created by doing:e [Command\ line]when no such file exists). Buffer name based heuristics are thus bound to run into annoying corner cases with files named outside the expected range. I know they have in my case, on occasion.
Let me know if this answers your question.
All of them are good points, good to be aware of Point 3. in particular.
Regarding Point 1., excluding nonmodifiable buffers may sometimes throw out the baby with the bathwater but, as a drawback, cf. Point 2., now buffers such as those by netrw have to be excluded by hand.
In conclusion, the exclusion rules of restore_view.vim are coarser and in corner cases even incorrect but perhaps an acceptable pragmatic compromise for those who shy away from finetuning their makeview exclusion list. It's good to know vim-stay does that already most of the time (but can never cover all kinds of buffer types plugin creators will come up with).
Actually, most plug-ins creating non-file buffers I have seen correctly mark their buffer as a non-empty buftype or set it up for wiping on hide, both cases vim-stay’s heuristics catch. Netrw is the one notable exception I am aware of (but if you find others that pass both the buftype and bufhidden check, I’ll gladly hear about them).
You are of course right that a blacklist based approach would not be sustainable, but that isn’t vim-stay’s solution: most everything is caught by checking other criteria, which explains why the default exclusion list currently contains all of 3 (you read that right: three) file types, two of which would also pass restore_view’s nomodifiable test. vim-stay is designed to work out of the box: you should not have to fine tune the exclusion list at all, and if you ever do, I want to hear about it so I can tweak the plug-in :).
@Konfekt BTW (related, but still somewhat OT): I like your FastFold plug-in. Let me know what I can do to make integration with vim-stay happen.
Ah, Danke für die Blumen.
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.
In conclusion, vim-stay integrates fine if loaded after FastFold, but don't touch that BufWinLeave autocmd :).
By the way, FastFold uses a variant of the MakeViewCheck() function of restore_view.vim to exclude non-modifiable buffers and alike (https://github.com/Konfekt/FastFold/blob/master/plugin/fastfold.vim#L170).
How about replacing it by the ispersistent() function of yours at https://github.com/kopischke/vim-stay/blob/master/autoload/stay.vim#L9 ? Any adaptions to be made?
EDIT: It's already out in the wild : Konfekt/FastFold@e7dee5a
How about replacing it by the ispersistent() function of yours at https://github.com/kopischke/vim-stay/blob/master/autoload/stay.vim#L9 ? Any adaptions to be made?
EDIT: It's already out in the wild : Konfekt/FastFold@e7dee5a
Nice, tho I’d suggest you remove the check for b:stay_ignore to decouple your function from vim-stay; you could insert your own “ignore” variable here, of course. Also, I think that (unlike vim-stay) you can safely ignore nomodifiablefiles – after all, the issue FastFold addresses only is present when editing files, right?
I’ve moved the discussion about integrating FastFold and vim-stay to its own issue: #5. Your thoughts would be appreciated :).
Hoppla, thanks for pointing out that obnoxious line. Fixed.
The nonmodifiable part was already taken account of in https://github.com/Konfekt/FastFold/blob/master/plugin/fastfold.vim#L171
Jo.
Heuristics have been tweaked in release 1.1.1, removing the need to list “netrw” among volatile file types. Generally speaking, there should be no need to list anything but bona fide file types that are transient by nature in g:volatile_ftypes, ever (covering-my-ass-caveat: grossly misbehaving plug-ins and hacks excepted – holler if you run into some ITW).
Thanks for the heads-up, passed that on to FastFold.
@Konfekt heuristics now include known temp directory checking based on Vim’s backupskip option as of release 1.2. Might want to check it out :).
Thanks! However, that's quite a beast for something that could look file
if path ~=# &backupskipif &backupskip wouldn't use a file pattern instead of regex pattern. Is there perhaps not a single function to convert between these?
join(map(split(&backupskip, ','), '"\V".escape(v:val, "\")'), '\|')no, wait, that doesn’t account for env vars, so
join(map(split(&backupskip, ','), '"\V".escape(expand(v:val), "\")'), '\|')actually, that needs to be anchored, so
join(map(split(&backupskip, ','), '"\V^".escape(expand(v:val), "\")'), '\|')– can’t say I find that much easier :).
Yeah, and on top for example * becomes .*...
If it's not in Vim it'll be quite some work. For example, the netrw#gitignore function tries to achieve the same and is a lot of code.
Yeah, and on top for example * becomes .*...
that‘s why there is an expand() call in there which also resolves the globs, but thanks for reminding me that produces multiple paths, so:
let dirs = []
for dirset in map(split(&backupskip, ','), 'expand(v:val, 0, 1)')
let dirs += dirset
endfor
return '\V^'.join(map(dirs,'escape(v:val, "\")'), '\|')and you’d also need to shim expand() because it doesn’t support List output before Vim 7.3.465. Now I am sure my way is easier :).
Ok. I asked at https://groups.google.com/forum/m/#!topic/vim_use/NXFksN223bc if there is something already in Vim as a last resort.
If not, the feature that FastFold skips temp files is sufficiently debatable that I'd leave it at check for the default value of &backupskip.
Hey, there's already a function in Vim that does this conversion and they are asking about the utility of making it available in Vimscript, perhaps you can think of other use cases than that which brought about the post at https://groups.google.com/forum/#!msg/vim_use/NXFksN223bc/vQG471egTkMJ
Interesting, and hats off to you for the effort. This being said, I can’t think of any use cases that make it preferable to vim-stay’s current approach. Even if you manage to get Bram to accept a patch, I’m not sure I would want to use that function, as it would not be backwards compatible with 99,9999 % of all Vim installations in existence for quite a while … never mind that it would land in 5 years or so at the earliest anyway :P (take a gander at the patch dates in todo.txt to see what I’m talking about; I’ve given up hoping for Vim patches to improve things beyond bugfixes and the private agenda of the core contributors).
Somehow glob2regexp() bypassed the todo list and is already alive and kicking since 7.4.668: https://groups.google.com/forum/#!msg/vim_use/NXFksN223bc/C74OfWY9wYsJ
Nice. Thanks for the heads up (though, as I said, I am not going to use this any time soon in a plug-in).
@Konfekt I’ve refactored stay#istemp() to be zippier. The result is faster by about factor 100 in my tests – and even faster when glob2regpat() is available, which really surprised me. I’ll retract my announcement about not using it, though it will have to be an optional variant for the foreseeable future.
Clever idea to filter by wildignore. I put its implementations on FastFold's todo list.
One advantage of having made glob2repat public is that its bugs now lie bare. For example over here it would not replace , by \|.
Clever idea to filter by
wildignore. I put its implementations on FastFold's todo list.
I’m pretty pleased with the performance myself, though I am also sure I will run into an edge case where that hack will fail spectacularly at some point :).
One advantage of having made
glob2repatpublic is that its bugs now lie bare. For example over here it would not replace,by\|.
If you mean the comma separating several globs in options like path, I’d surmise this is by design; after all, these are technically several globs, not one. splitting and mapping should take care of that.
Ok, I see that split(&backupskip, '\m[^\\]\%(\\\\\)*,') takes care of that.
A question: My naive try would have been split(&backupskip, ','). Why is [^\\]\%(\\\\\)* necessary ?
Weil, \, is an escaped comma, i.e. one that is part of the glob, but \\, is a trailing backslash, of which there can be any number, and everything needs to be doubled to be a literal backslash in a regex pattern (well, a single quoted one: you would need a quadruple backslash inside double quotes – fun, eh?). I may be overthinking what probably is a pretty rare edge case, of course :).
Edit: thinking this through, I noticed the regex will munge the first glob in a list starting with a comma (i.e. the first non empty item); [^\\] should actually be \\\@<!.
Why was this reopened? Mis-click? 😄
Butterfingers. 🤗