Persistent state

[intarga]

Resolves part of #401, along with #9154

TODO LIST:

• persist command history
• persist search history
• persist file history
• persist clipboard contents
  - [ ] persist splits
  - [ ] load in background tasks
• command to manually reload history
• handle errors nicely
• trim history files
• config options for persistence (on/off)
• config options for persistence (limits)
• config options for persistence (excluded files)
• testing
• documentation

[kirawi]

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.

[kirawi]

However, if this is tackling persistent undo as well, then I believe @pascalkuthe wanted to avoid using serde.

[intarga]

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.

[pascalkuthe]

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

[intarga]

I have no particular attachment to serde or messagepack, so I'll happily switch to bincode.

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.

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:

1. Entries from different contemporary sessions can be shown in chronological order, instead of the order their sessions quit.
2. Entry types where it only makes sense to have a single entry (clipboard contents and split layout, perhaps) can avoid duplication, and can choose the last entry chronologically.
3. Length limits for different entry types can be enforced separately (i.e. limit oldfiles to 500 entries, but command history to 1000).
4. Spamming entries of one type does not risk evicting all entries of another (i.e. if I send a ton of commands, I don't risk losing all my oldfiles).
5. Entries of the same type can stay clustered, which makes reading easier and more flexible.
6. There's a straight forward answer to when and how to trim the file. If we go append-only, it's not clear what to do. If we're enforcing a limit of X entries, then surely that means we have to trim every time we write. Unless I'm missing something, trimming will have to involve at least partly reading and deserialising the file so we can figure out what to remove. If we're reading the file every time we write, I'm not sure I see a meaningful benefit to append-only over merging.

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 🤷‍♀️

[pascalkuthe]

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.

[intarga]

Ok, I'll have a go at the multi-file approach then 👍

[gyreas]

If you need an alternative name to ShaDa, can you consider Hexion (Helix session)?

It's tongue-in-check tho

[intarga]

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

[gabydd]

I think the aligning when switching might actually be a bug, cause it can cause a lot of shifting when doing something like ga (going back and forth to the last accessed buffer)

[intarga]

@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

[gabydd]

Ah okay I'll try to take a better look when I have the time

[intarga]

    fn replace_document_in_view(&mut self, current_view: ViewId, doc_id: DocumentId) {
        let view = self.tree.get_mut(current_view);
        view.doc = doc_id;
        view.offset = ViewPosition::default();

        let doc = doc_mut!(self, &doc_id);
        doc.ensure_view_init(view.id);
        view.sync_changes(doc);
        doc.mark_as_focused();

        align_view(doc, view, Align::Center);
    }

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 ViewPosition information in Document, since it currently only seems to have information about the selections.

[the-mikedavis]

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.

[intarga]

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.

[intarga]

@useche Apologies for the late response.

I was having an issue (which I don't remember) with the fact that they were initialized so early. In any case, even now some reasons come to mind: (1) I'm not a fan of global variables and the persistent state location is something that only the persistent code cares about, (2) it's nice to have related code close by, (3) the persistent state location might now change with :reload-config and I didn't feel that comfortable changing the global variables at this point. I see that they should be synchronized, though, so that technically shouldn't be a problem. Let me know your thoughts.

My opposition to this was only that I think we should try to stick to existing conventions where possible. (3) seems like a good reason to break the convention though.

I'm not sure whether we should eventually mix those with this pull request or rather wait until this is checked in and then create a new pull request with those. Let me know if you have an opinion about that.

I think it would be best to have split persistence in a follow on PR to make things easier for reviewers, as this one is already quite big.

Continuing our conversation, I implemented a new way to set the persistence options. I wanted to simplify it based on the conversations we had. Now my configuration looks like:

[editor.persistence]
all.enabled = true
all.max-entries = 1000
all.scope = "per-workspace"
autostart-splits = true

The option all will be the default for all the specific options. Specific options (like "search" or "commands") can be changed for more specific options. The patch is in the top of my github tree useche/helix. Let me know what you think.

I'm not sure I see the point in all.max-entries since the content of the entries is so different. I also don't think all.enabled and all.scope should be separate; I would prefer all = "global"|all = "workspace"|all = "off". And perhaps default would be a more intuitive name than all.

On another note, it would be good to have integration testing of per-workspace persistence

[intarga]

@Axlefublr It's rebased on master now

[Axlefublr]

@intarga thank you so much! and what wonderful timing, I was just in the process of git magicking my fork to add a new feature while being on an older commit; now I can just rebase on master like normal :D

[ThanHenderson]

Thanks a lot for this. Is there a timeline to merge here?

[intarga]

Thanks a lot for this. Is there a timeline to merge here?

It's up to the maintainers now to review it. I have no idea where this fits on their priority list, I imagine they have a lot on their plates.

[Axlefublr]

can you please rebase on master? there are quite a few merge conflicts, that I couldn't figure out how to handle myself

[gabydd]

I have it merged with master here gabydd@eca36b7

[Axlefublr]

@gabydd thank you! I ended up figuring things out though. however the more rarely a pr is rebased on master, the more convoluted trying to figure things out is going to be, so that's the main reason I ask