Skip to content
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

Commit undo checkpoints before each :write #11062

Merged
merged 1 commit into from
Jul 13, 2024
Merged

Conversation

the-mikedavis
Copy link
Member

@the-mikedavis the-mikedavis commented Jun 30, 2024

This fixes the modification indicator when saving from insert mode with a config such as:

[keys.insert]
C-s = ":write"

Previously the modification indicator would be stuck showing modified even if the buffer contents matched the disk contents when writing after some changes in insert mode with this binding. In insert mode we do not eagerly write undo checkpoints so that all changes made become one checkpoint as you exit insert mode. When saving, Documents changes ChangeSet would be non-empty and when there are changes we show the buffer as modified. Then switching to normal mode would append the changes to history, bumping the current revision past what it was when last saved. Since the last saved revision and current revision were then unsynced, the modification indicator would always show modified.

This matches Kakoune's behavior. Kakoune has a different architecture for writes but a very similar system for history, transactions and undo checkpoints (what it calls "undo groups"). Upon saving Kakoune creates an undo checkpoint if there are any uncommitted changes. It does this after the write has gone through since its writing system is different. For our writing system it's cleaner to make the undo checkpoint before performing the save so that the history revision increments before we send the save event.

Fixes #11024
Closes #7226

I have also reverted #11047 in this PR since delaying :writes until leaving insert mode is no longer necessary. I believe that not saving in insert mode weakens the auto-save-after-delay feature considerably.

This fixes the modification indicator when saving from insert mode with
a config such as

    [keys.insert]
    C-s = ":write"

Previously the modification indicator would be stuck showing modified
even if the buffer contents matched the disk contents when writing after
some changes in insert mode with this binding. In insert mode we do not
eagerly write undo checkpoints so that all changes made become one
checkpoint as you exit insert mode. When saving, `Document`s `changes`
`ChangeSet` would be non-empty and when there are changes we show the
buffer as modified. Then switching to normal mode would append the
changes to history, bumping the current revision past what it was when
last saved. Since the last saved revision and current revision were then
unsynced, the modification indicator would always show modified.

This matches [Kakoune's behavior]. Kakoune has a different architecture
for writes but a very similar system for history, transactions and undo
checkpoints (what it calls "undo groups"). Upon saving Kakoune creates
an undo checkpoint if there are any uncommitted changes. It does this
after the write has gone through since its writing system is different.
For our writing system it's cleaner to make the undo checkpoint before
performing the save so that the history revision increments before we
send the save event.

[Kakoune's behavior]: https://github.com/mawww/kakoune/blob/80fcfebca8c62ace6cf2af9487784486af07d2d5/src/buffer.cc#L565-L566
@the-mikedavis the-mikedavis added C-bug Category: This is a bug A-command Area: Commands labels Jun 30, 2024
}

// Save an undo checkpoint for any outstanding changes.
doc.append_changes_to_history(view);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally Document::save_impl would be in charge of this but currently Document::append_changes_to_history needs a &mut View. Maybe we can refactor that later and move this call into document.

@the-mikedavis
Copy link
Member Author

On second though I dropped the commit to revert #11047. @pascalkuthe and I discussed it and reverting it would mean making potentially many of undo checkpoints in insert mode. If we can find a clever way to track modification status without undo checkpoints we could restore the "save in insert mode" behavior for auto-save.

@archseer archseer merged commit 44d2fc2 into master Jul 13, 2024
6 checks passed
@archseer archseer deleted the commit-undo-on-write branch July 13, 2024 17:59
mxxntype pushed a commit to mxxntype/helix that referenced this pull request Aug 14, 2024
This fixes the modification indicator when saving from insert mode with
a config such as

    [keys.insert]
    C-s = ":write"

Previously the modification indicator would be stuck showing modified
even if the buffer contents matched the disk contents when writing after
some changes in insert mode with this binding. In insert mode we do not
eagerly write undo checkpoints so that all changes made become one
checkpoint as you exit insert mode. When saving, `Document`s `changes`
`ChangeSet` would be non-empty and when there are changes we show the
buffer as modified. Then switching to normal mode would append the
changes to history, bumping the current revision past what it was when
last saved. Since the last saved revision and current revision were then
unsynced, the modification indicator would always show modified.

This matches [Kakoune's behavior]. Kakoune has a different architecture
for writes but a very similar system for history, transactions and undo
checkpoints (what it calls "undo groups"). Upon saving Kakoune creates
an undo checkpoint if there are any uncommitted changes. It does this
after the write has gone through since its writing system is different.
For our writing system it's cleaner to make the undo checkpoint before
performing the save so that the history revision increments before we
send the save event.

[Kakoune's behavior]: https://github.com/mawww/kakoune/blob/80fcfebca8c62ace6cf2af9487784486af07d2d5/src/buffer.cc#L565-L566
kyruzic pushed a commit to kyruzic/helix that referenced this pull request Sep 27, 2024
This fixes the modification indicator when saving from insert mode with
a config such as

    [keys.insert]
    C-s = ":write"

Previously the modification indicator would be stuck showing modified
even if the buffer contents matched the disk contents when writing after
some changes in insert mode with this binding. In insert mode we do not
eagerly write undo checkpoints so that all changes made become one
checkpoint as you exit insert mode. When saving, `Document`s `changes`
`ChangeSet` would be non-empty and when there are changes we show the
buffer as modified. Then switching to normal mode would append the
changes to history, bumping the current revision past what it was when
last saved. Since the last saved revision and current revision were then
unsynced, the modification indicator would always show modified.

This matches [Kakoune's behavior]. Kakoune has a different architecture
for writes but a very similar system for history, transactions and undo
checkpoints (what it calls "undo groups"). Upon saving Kakoune creates
an undo checkpoint if there are any uncommitted changes. It does this
after the write has gone through since its writing system is different.
For our writing system it's cleaner to make the undo checkpoint before
performing the save so that the history revision increments before we
send the save event.

[Kakoune's behavior]: https://github.com/mawww/kakoune/blob/80fcfebca8c62ace6cf2af9487784486af07d2d5/src/buffer.cc#L565-L566
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-command Area: Commands C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using :w as shortcut in insert mode leaves buffer with unsaved marker
3 participants