-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Persistent state #9143
base: master
Are you sure you want to change the base?
Persistent state #9143
Conversation
Are you interested in working on buffer state, history, or both? I was going to get back to persistent undo this week, but if you intend to tackle it then I'll leave it to you. |
However, if this is tackling persistent undo as well, then I believe @pascalkuthe wanted to avoid using serde. |
Hey @kirawi! This is not tackling undo, I looked at your PR and I don’t think there’s any overlap. I’m going to make a detailed post for discussion on the issue once I have a working protype, but I’ll write up some of my findings here now in case they’re helpful: I looked into Neovim’s ShaDa (one single file in MessagePack format), following archseer’s suggestion. It’s used to persist most of what we want except notably undo history, and there seem to be some good reasons for that. For one thing, undo histories get big very quickly, and if we want to save a significant amount of history for a significant number of files, they will bloat the shada file probably more than we want. Neovim instead seems to have one undo file per edited file, which seems like a better approach, though I couldn’t find any info on the details of the format for their undofiles. From this though, I think it makes sense for persistent undo to be implemented separately from the rest. |
So the problem I have with serde is that it doesn't allow incremental or streaming serialization and deserialization. I think for undofile having that would be nice but not a hard requirement. (N)vim has a pretty simple file format and simply fully writes/reads the undofile whenever it writes/reads the file. I bieve we could do better by changing the format a bit so we only append whenever we save (and make it easy to evict old revsions). But I am not set on that. If we comeup with a scheme to instead evict in memory, keep the undofile fairly small writing on each save (just like vim) may be ok and then we could use serde/bincode (very fast and mature serialization format). For undofiles the ultimate question is probably garbage collection. How do we keep the u dofile from growing forever. For session data/something like shada in vim I do like the idea of doing something similar vht here again append only serialization and streaming deserialization are important. That wknt work well with serde. The pvject headers are simple e ought to write your own parser (no need for msgpack). They are fully seldescibing and describe the size of some arbitrary data that follows the header. We can read that data and deserialize with serde (And don't need msgpack I prefer bincode). |
I have no particular attachment to serde or messagepack, so I'll happily switch to bincode.
I'm not sure I agree about "append only". I see the benefit that it would make simple writes faster, but at the cost of flexibility and ease in reading and trimming. The design of shada in nvim seems to at least initially have been motivated by append-only writes, which is why it's a concat of msgpack objects instead of an array, but it seems like they ultimately decided not to follow through with this and instead they merge with the existing file when writing. While merging is presumably more expensive than appending, it has some notable advantages:
Some, but not all, of these problems could be solved by having dedicated files for different entry types, but then you have to write a bunch of files instead of just one 🤷♀️ |
d22bfcd
to
4987e1a
Compare
I personally always thought that having multiple files would be the way to go. Especially for command history that should work well. That could work more or less the same as zsh history (it's 3xactly the same concept). I don't really think it's a problem having multiple datafiles. I think a big advantage of appendonly is that you avoid frequently writing large amounts of data to disk. It's not a huge deal but reducing background io is nice. For other files like registers I agree that it doesn't make sense to have appendonly files. But I would just keep these entirely separate. I never understood why nvim went with a single file model it seems to make everything (including the merging) much more complicated without much tangiböe benefit. |
Ok, I'll have a go at the multi-file approach then 👍 |
c3c5be0
to
9d53156
Compare
If you need an alternative name to ShaDa, can you consider Hexion (Helix session)? It's tongue-in-check tho |
@the-mikedavis When opening files with their saved position, should the view be aligned to centre? Although I would personally prefer to not align, I noticed that helix aligns to centre even on buffer switches, so I'm assuming that's the convention, and in that case we only need to persist the selection, not the view. Should I stick with that? Should alignment be configurable? |
I think the aligning when switching might actually be a bug, cause it can cause a lot of shifting when doing something like |
@gabydd I also don't like that behaviour, though I don't think it's a bug. The code for it looks pretty intentional, and vim notably seems to behave the same |
Ah okay I'll try to take a better look when I have the time |
I'm not completely sure, but I think this is the relevant function, and that alignment looks pretty intentional. If I get approval I would very much like to change this behaviour. Though I think to remove the shifting on buffer switched we might have to persist |
There was a PR about this that I think is not yet finished and could probably be picked up and brought across the finish line if you're interested: #7414 I would prefer that we save the View's offset (ViewPosition) and use it if possible, centering only if the cursor wouldn't be visible. |
I would gladly take that on! It seems though that the work might already be done. I found this PR #7568 by the same author which seems to address the issues raised in that thread, and is waiting on review. The author mentions an unresolved issue, but looking at the code, I think that was just a misunderstanding. |
7e98d28
to
45b7c16
Compare
a029c78
to
cb48389
Compare
7a5d8ff
to
3e13a61
Compare
I've pushed the fix. @sibouras let me know if it works for you |
yayy it works great!! thanks |
I'm trying to use this I've compiled it without issue on Ubuntu 22.04 and it accepts these options:
But I don't understand what to do in the session to make it persist, is there a command I run or is it supposed to do it automatically and it's not for some reason? I have the reload-history command but it doesn't seem to do anything and I can't seem to find another one |
@agieocean It is intended to work automatically. What are you expecting to happen that doesn't? |
My apologies I read back through the work you did and realized I misunderstood something, good work |
3231f29
to
451de43
Compare
Hello. Would it be possible to add a bit more info to the missing items in the to-do list? I mean if you need help with that, so that potentially interested people have an easier starting point. Thanks |
It would be nice if the command history would match the text typed, for example: Also it would be cool to have something like nvim sessions plugins. When you can restore all of your buffers from the last session/previous sessions. And the list of the last opened files too. The recent files feature from nvim's telescope plugin. Thanks! |
@davidrios Are you offering to submit a patch? |
That sounds like a useful feature, but it's unrelated to this PR. command history actually already existed before, and I haven't changed the way it works, just persisted it between sessions. If there doesn't already exist an issue for this, you should make one. Better yet, if you're feeling brave, I would encourage you to have a go at implementing it 🙂
That was my intention with the "persist splits" task, but I decided not to implement it in this PR, because the PR is already quite big, and that feature is different enough from the rest I think it deserves its own treatment. I also think it would be better implemented by someone who actually uses splits and would use this feature, which I don't and wouldn't. I would be happy to offer my help/context/advice to anyone who wants to have a go at implementing this.
The work I've done in this PR makes this pretty low-hanging fruit, but again, I think it deserves it's own PR. I might pick this one up myself after merging the current PR though |
You are awesome @intarga, thank you! |
@intarga |
@intarga Yeah pretty much, if you don't mind. Doesn't need to be anything detailed, just the gist, something like "testing: add 100% unit test coverage" and "handle errors nicely: remove all unwraps". |
@davidrios I would be glad to receive a patch! With the errors you're mostly on the money; there are a lot of unwraps/panics that should be recoverable, and either raise an error message to the user, and/or put something in the log. In terms of tests I'm not sure there's much value in unit tests for this (though it never hurts to have them), my intention was to write an integration test. In this commit I fixed the existing integration tests to work with persistence using tempfiles. This was useful to catch panics, but it does not actually verify the correctness of the persisted state. Ideally we should add an integration test that opens and closes some files, sends some commands, does some searches, ends the session, then starts a new session and checks the state is all loaded correctly. |
@davidrios I had some time and energy to write the integration test, so that no longer needs doing |
@davidrios Aaaaand I fixed the error handling too 😅 (though I actually decided to keep pretty much all the unwraps) |
This is finally ready for review 🎉 A couple of notes for the reviewers:
|
Resolves part of #401, along with #9154
TODO LIST:
- [ ] persist splits- [ ] load in background tasks