Persistent Undo

[kirawi]

Objective

Implement persistent undo as part of #401

Solution

• Implemented parser combinators in helix-core::parse.
• Implemented a serialized file format for History. At the moment, timestamps aren't preserved.
  • New revisions are appended, and the tree is reconstructed during deserialization.
• Added new quickcheck tests for serialization/deserialization/merging histories, and an integration test.
• :reload will also merge the saved history with the current one.
• Enabling persistent-undo at runtime will cause the histories of open docs to be merged as well.

---

Changelog

implemented persistent undo

[pascalkuthe]

@pascalkuthe suggested appending the absolute paths of the files to the session directory, but Rust doesn't allow this: rust-lang/rust#16507. Implementing a function with that behavior seemed more work than an index.

That's not quite what I meant. What I meant instead is that there is some central location (notably not a session but rather somewhere central. For example nvim default to $XDG_STATE_HOME/nvim/undo/. The location doesn't matter much and could be configurable (nvim also allows this by setting the undodir option).

Ofcourse you can not joint the absolute paths on top of a directory. However you can escape a path.
For example the undo file for /home/pascalkuthe/Projects/helix/README.md would be ~/$XDG_STATE_HOME/helix/undo/'%home%pascalkuthe%Projects%helix%README.md' so the entire filepath just becomes a filename. Rust is actually very well suited for implementing something like this. You can reuse the Path::components iterator for that which takes care of the hard part of splitting the file into componenents for you.

I am also of the opinion that the undofile should be (if enabled) automatically written on save and automatically opened when a file is opened instead of manually opened from a session.

In general undofiles are seperate from sessions to me. Just like in nvim undofiles should be seperate because they are decidedly not session specific. Each file has one history of changes no matter what session performs these changes (which is why I think it's better to autoamtically save and load undofiles from a centralized location instead of manual load from a specific session). With such a global approach the indexfile becomes impractical. There would be laods of conflict and dataloss for that index file between different helix instances whereas for standalone escaped undofiles only have contention when two helix instances write to the same file (in which case there is already contention anyway which is a seperate problem to solve with file watching/mtime safeguard)

[kirawi]

Yeah, that seems like an overall improvement and not too big of a change. One thing I liked about workspaces in VSCode (but forgot to implement here) is that it would open the last open buffers. Maybe something like that could be done by looking for undofiles beginning with the cwd path, and having .undo.last extension?

[antoyo]

I'm also for having undo-files separate from sessions since I plan to use sessions, but not undo-files.

[pascalkuthe]

Yeah, that seems like an overall improvement and not too big of a change. One thing I liked about workspaces in VSCode (but forgot to implement here) is that it would open the last open buffers. Maybe something like that could be done by looking for undofiles beginning with the cwd path, and having .undo.last extension?

I think that is a session related feature. Like I said I think sessions and undofiles are separate features. So the undofiles always get opened and saved when the files do (if the setting is enabled). However open buffers only get restored when a session is opened (would just be a list of paths stored in the session file).

These two files have seperate lifetimes and I think mixing them will only lead to problems.

[kirawi]

I've implemented reloading the history on :reload and appending the history.

[SollyCB]

Idk if I am missing something, I will assume no reply means I am not in the know. And I am not sure what was decided on loading the undo file into memory. But:

I understand undos to work as operations pushed to a Vec.
If when a change is made, it is pushed to the list of Operations, can't this list be written to disk? Along with a hash of the file? When helix opens a file, the hashes are compared with the current file, if hashes do not match (file was edited by another program) the undo list is invalidated and is emptied. Oldest changes are then the changes made from this point on. If hashes do match and the files are the same then the undo list is loaded as the current Vec.

Regarding the same file being edited by different instances of helix, can't the timestamp of the most recent operation be saved also, and whichever list has the most recent timestamp be the one that is maintained? So if instance A of helix writes the file that is open and therefore writes to the undo list, when instance B goes to save it can see that it is not at the most recent version of the file and can open the most recent version as a swapfile to compare changes (which also loads the newest version of the undo list), or overwrites its version of the file with the most recent one, which also overwrites its version of the undo list that it has loaded into memory.

I think I am largely reiterating PascalKuthe's comments. But I feel the tree is unnecessary: simply load the undo list into memory upon opening a file, overwrite the list on save.

Again apologies if this is a timewasting comment, I am a noob desperate to learn.

[kirawi]

See #5608 (comment)

The undo history is already a tree as well.

[kirawi]

The test for reloading don't seem sufficient, since I ran into a case where it panics. This was the revisions deserialized so far, and the revision that had an invalid parent:

    Revision {
        parent: 1,
        last_child: None,
        transaction: Transaction {
            changes: ChangeSet {
                changes: [
                    Retain(
                        6998,
                    ),
                    Delete(
                        2,
                    ),
                    Retain(
                        23442,
                    ),
                ],
                len: 30442,
                len_after: 30440,
            },
            selection: Some(
                Selection {
                    ranges: [
                        Range {
                            anchor: 6998,
                            head: 6999,
                            old_visual_position: None,
                        },
                    ],
                    primary_index: 0,
                },
            ),
        },
        inversion: Transaction {
            changes: ChangeSet {
                changes: [
                    Retain(
                        6998,
                    ),
                    Insert(
                        "md",
                    ),
                    Retain(
                        23442,
                    ),
                ],
                len: 30440,
                len_after: 30442,
            },
            selection: Some(
                Selection {
                    ranges: [
                        Range {
                            anchor: 7000,
                            head: 7001,
                            old_visual_position: Some(
                                (
                                    0,
                                    13,
                                ),
                            ),
                        },
                    ],
                    primary_index: 0,
                },
            ),
        },
        timestamp: Instant {
            tv_sec: 21118,
            tv_nsec: 598440833,
        },
    },
]

Revision {
    parent: 2,
    last_child: None,
    transaction: Transaction {
        changes: ChangeSet {
            changes: [
                Retain(
                    6998,
                ),
                Insert(
                    "md",
                ),
                Retain(
                    23442,
                ),
            ],
            len: 30440,
            len_after: 30442,
        },
        selection: Some(
            Selection {
                ranges: [
                    Range {
                        anchor: 7001,
                        head: 7000,
                        old_visual_position: None,
                    },
                ],
                primary_index: 0,
            },
        ),
    },
    inversion: Transaction {
        changes: ChangeSet {
            changes: [
                Retain(
                    6998,
                ),
                Delete(
                    2,
                ),
                Retain(
                    23442,
                ),
            ],
            len: 30442,
            len_after: 30440,
        },
        selection: Some(
            Selection {
                ranges: [
                    Range {
                        anchor: 6999,
                        head: 6998,
                        old_visual_position: None,
                    },
                ],
                primary_index: 0,
            },
        ),
    },
    timestamp: Instant {
        tv_sec: 21118,
        tv_nsec: 598440833,
    },
}

I'm concerned about the apparently missing revisions, especially since all histories at least have an empty initial revision which should have been deserialized too.

[kirawi]

I think I traced the cause of the bug to how the history is saved in Document::save_impl. If another Helix instance had written to the undofile, then the current instance attempts to overwrite the entire file with that doc's history, but will pass the same range of revisions as if it was appending.

[robrecord]

Thanks so much for your work on this @kirawi!

[kirawi]

Will be superseded by a new PR.

[fastfading]

is this issue fixed ? what is the conclusion ?
this is the basic funtion of an edit.
without it. I wil still use neovim although I like the design of helix

[johannesrave]

@kirawi which PR is the new one to follow for this? thanks!

i think i found it, sorry #9154

[cole-h]

I'd venture to guess: #9154