Fix: Undo and redo correctly updates editor modified status (#244)

* Set an index for the last saved change

I added an index that represents the last saved change.

Editors are considered to be unsaved or modified if the current change
is different from the save index. In other words, if the last saved
change is `5`, undoing or redoing past that change should indicate that
the editor has been modified.

This is needed to fix two bugs in COSMIC Edit:
* pop-os/cosmic-edit#116
* pop-os/cosmic-edit#128

* Unit test that confirms pivot logic works

I'll most likely simplify the API as end users don't have a way to
cleanly use `Pivot::Exact` without access to the internal command
buffer.

* Simplify save point API

* Implement more save point unit tests

A unit test for an edge case currently fails but normal usage works.

* Fix edge case for empty command index and pivot

* More save point unit tests for common use cases

Author: joshuamegnauth54

src/edit/vi.rs

@@ -31,20 +31,39 @@ fn finish_change<'buffer, E: Edit<'buffer>>(
     editor: &mut E,
     commands: &mut cosmic_undo_2::Commands<Change>,
     changed: &mut bool,
+    pivot: Option<usize>,
 ) -> Option<Change> {
     //TODO: join changes together
     match editor.finish_change() {
         Some(change) => {
             if !change.items.is_empty() {
                 commands.push(change.clone());
-                *changed = true;
+                *changed = eval_changed(commands, pivot);
             }
             Some(change)
         }
         None => None,
     }
 }
 
+/// Evaluate if an [`ViEditor`] changed based on its last saved state.
+fn eval_changed(commands: &cosmic_undo_2::Commands<Change>, pivot: Option<usize>) -> bool {
+    // Editors are considered modified if the current change index is unequal to the last
+    // saved index or if `pivot` is None.
+    // The latter case handles a never saved editor with a current command index of None.
+    // Check the unit tests for an example.
+    match (commands.current_command_index(), pivot) {
+        (Some(current), Some(pivot)) => current != pivot,
+        // Edge case for an editor with neither a save point nor any changes.
+        // This could be a new editor or an editor without a save point where undo() is called
+        // until the editor is fresh.
+        (None, None) => false,
+        // Default to true because it's safer to assume a buffer has been modified so as to not
+        // lose changes
+        _ => true,
+    }
+}
+
 fn search<'buffer, E: Edit<'buffer>>(editor: &mut E, value: &str, forwards: bool) -> bool {
     let mut cursor = editor.cursor();
     let start_line = cursor.line;
@@ -167,6 +186,7 @@ pub struct ViEditor<'syntax_system, 'buffer> {
     search_opt: Option<(String, bool)>,
     commands: cosmic_undo_2::Commands<Change>,
     changed: bool,
+    save_pivot: Option<usize>,
 }
 
 impl<'syntax_system, 'buffer> ViEditor<'syntax_system, 'buffer> {
@@ -179,6 +199,7 @@ impl<'syntax_system, 'buffer> ViEditor<'syntax_system, 'buffer> {
             search_opt: None,
             commands: cosmic_undo_2::Commands::new(),
             changed: false,
+            save_pivot: None,
         }
     }
 
@@ -233,6 +254,20 @@ impl<'syntax_system, 'buffer> ViEditor<'syntax_system, 'buffer> {
         self.changed = changed;
     }
 
+    /// Set current change as the save (or pivot) point.
+    ///
+    /// A pivot point is the last saved index. Anything before or after the pivot indicates that
+    /// the editor has been changed or is unsaved.
+    ///
+    /// Undoing changes down to the pivot point sets the editor as unchanged.
+    /// Redoing changes up to the pivot point sets the editor as unchanged.
+    ///
+    /// Undoing or redoing changes beyond the pivot point sets the editor to changed.
+    pub fn save_point(&mut self) {
+        self.save_pivot = Some(self.commands.current_command_index().unwrap_or_default());
+        self.changed = false;
+    }
+
     /// Set passthrough mode (true will turn off vi features)
     pub fn set_passthrough(&mut self, passthrough: bool) {
         if passthrough != self.passthrough {
@@ -251,19 +286,17 @@ impl<'syntax_system, 'buffer> ViEditor<'syntax_system, 'buffer> {
         log::debug!("Redo");
         for action in self.commands.redo() {
             undo_2_action(&mut self.editor, action);
-            //TODO: clear changed flag when back to last saved state?
-            self.changed = true;
         }
+        self.changed = eval_changed(&self.commands, self.save_pivot);
     }
 
     /// Undo a change
     pub fn undo(&mut self) {
         log::debug!("Undo");
         for action in self.commands.undo() {
             undo_2_action(&mut self.editor, action);
-            //TODO: clear changed flag when back to last saved state?
-            self.changed = true;
         }
+        self.changed = eval_changed(&self.commands, self.save_pivot);
     }
 
     #[cfg(feature = "swash")]
@@ -550,7 +583,12 @@ impl<'syntax_system, 'buffer> Edit<'buffer> for ViEditor<'syntax_system, 'buffer
     }
 
     fn finish_change(&mut self) -> Option<Change> {
-        finish_change(&mut self.editor, &mut self.commands, &mut self.changed)
+        finish_change(
+            &mut self.editor,
+            &mut self.commands,
+            &mut self.changed,
+            self.save_pivot,
+        )
     }
 
     fn action(&mut self, font_system: &mut FontSystem, action: Action) {
@@ -564,7 +602,12 @@ impl<'syntax_system, 'buffer> Edit<'buffer> for ViEditor<'syntax_system, 'buffer
         if self.passthrough {
             editor.action(font_system, action);
             // Always finish change when passing through (TODO: group changes)
-            finish_change(editor, &mut self.commands, &mut self.changed);
+            finish_change(
+                editor,
+                &mut self.commands,
+                &mut self.changed,
+                self.save_pivot,
+            );
             return;
         }
 
@@ -589,7 +632,12 @@ impl<'syntax_system, 'buffer> Edit<'buffer> for ViEditor<'syntax_system, 'buffer
                 log::debug!("Pass through action {:?}", action);
                 editor.action(font_system, action);
                 // Always finish change when passing through (TODO: group changes)
-                finish_change(editor, &mut self.commands, &mut self.changed);
+                finish_change(
+                    editor,
+                    &mut self.commands,
+                    &mut self.changed,
+                    self.save_pivot,
+                );
                 return;
             }
         };
@@ -620,7 +668,12 @@ impl<'syntax_system, 'buffer> Edit<'buffer> for ViEditor<'syntax_system, 'buffer
                     return;
                 }
                 Event::ChangeFinish => {
-                    finish_change(editor, &mut self.commands, &mut self.changed);
+                    finish_change(
+                        editor,
+                        &mut self.commands,
+                        &mut self.changed,
+                        self.save_pivot,
+                    );
                     return;
                 }
                 Event::Delete => Action::Delete,
@@ -687,7 +740,12 @@ impl<'syntax_system, 'buffer> Edit<'buffer> for ViEditor<'syntax_system, 'buffer
                                 }
                             }
                         }
-                        finish_change(editor, &mut self.commands, &mut self.changed);
+                        finish_change(
+                            editor,
+                            &mut self.commands,
+                            &mut self.changed,
+                            self.save_pivot,
+                        );
                     }
                     return;
                 }

tests/editor_modified_state.rs

@@ -0,0 +1,227 @@
+#![cfg(feature = "vi")]
+
+use std::sync::OnceLock;
+
+use cosmic_text::{Buffer, Cursor, Edit, Metrics, SyntaxEditor, SyntaxSystem, ViEditor};
+
+static SYNTAX_SYSTEM: OnceLock<SyntaxSystem> = OnceLock::new();
+
+// New editor for tests
+fn editor() -> ViEditor<'static, 'static> {
+    // More or less copied from cosmic-edit
+    let font_size: f32 = 14.0;
+    let line_height = (font_size * 1.4).ceil();
+
+    let metrics = Metrics::new(font_size, line_height);
+    let buffer = Buffer::new_empty(metrics);
+    let editor = SyntaxEditor::new(
+        buffer,
+        SYNTAX_SYSTEM.get_or_init(SyntaxSystem::new),
+        "base16-eighties.dark",
+    )
+    .expect("Default theme `base16-eighties.dark` should be found");
+
+    ViEditor::new(editor)
+}
+
+// Tests that inserting into an empty editor correctly sets the editor as modified.
+#[test]
+fn insert_in_empty_editor_sets_changed() {
+    let mut editor = editor();
+
+    assert!(!editor.changed());
+    editor.start_change();
+    editor.insert_at(Cursor::new(0, 0), "Robert'); DROP TABLE Students;--", None);
+    editor.finish_change();
+    assert!(editor.changed());
+}
+
+// Tests an edge case where a save point is never set.
+// Undoing changes should set the editor back to unmodified.
+#[test]
+fn insert_and_undo_in_unsaved_editor_is_unchanged() {
+    let mut editor = editor();
+
+    assert!(!editor.changed());
+    editor.start_change();
+    editor.insert_at(Cursor::new(0, 0), "loop {}", None);
+    editor.finish_change();
+    assert!(editor.changed());
+
+    // Undoing the above change should set the editor as unchanged even if the save state is unset
+    editor.start_change();
+    editor.undo();
+    editor.finish_change();
+    assert!(!editor.changed());
+}
+
+#[test]
+fn undo_to_save_point_sets_editor_to_unchanged() {
+    let mut editor = editor();
+
+    // Latest saved change is the first change
+    editor.start_change();
+    let cursor = editor.insert_at(Cursor::new(0, 0), "Ferris is Rust's ", None);
+    editor.finish_change();
+    assert!(
+        editor.changed(),
+        "Editor should be set to changed after insertion"
+    );
+    editor.save_point();
+    assert!(
+        !editor.changed(),
+        "Editor should be set to unchanged after setting a save point"
+    );
+
+    // A new insert should set the editor as modified and the pivot should still be on the first
+    // change from earlier
+    editor.start_change();
+    editor.insert_at(cursor, "mascot", None);
+    editor.finish_change();
+    assert!(
+        editor.changed(),
+        "Editor should be set to changed after inserting text after a save point"
+    );
+
+    // Undoing the latest change should set the editor to unmodified again
+    editor.start_change();
+    editor.undo();
+    editor.finish_change();
+    assert!(
+        !editor.changed(),
+        "Editor should be set to unchanged after undoing to save point"
+    );
+}
+
+#[test]
+fn redoing_to_save_point_sets_editor_as_unchanged() {
+    let mut editor = editor();
+
+    // Initial change
+    assert!(
+        !editor.changed(),
+        "Editor should start in an unchanged state"
+    );
+    editor.start_change();
+    editor.insert_at(Cursor::new(0, 0), "editor.start_change();", None);
+    editor.finish_change();
+    assert!(
+        editor.changed(),
+        "Editor should be set as modified after insert() and finish_change()"
+    );
+    editor.save_point();
+    assert!(
+        !editor.changed(),
+        "Editor should be unchanged after setting a save point"
+    );
+
+    // Change to undo then redo
+    editor.start_change();
+    editor.insert_at(Cursor::new(1, 0), "editor.finish_change()", None);
+    editor.finish_change();
+    assert!(
+        editor.changed(),
+        "Editor should be set as modified after insert() and finish_change()"
+    );
+    editor.save_point();
+    assert!(
+        !editor.changed(),
+        "Editor should be unchanged after setting a save point"
+    );
+
+    editor.undo();
+    assert!(
+        editor.changed(),
+        "Undoing past save point should set editor as changed"
+    );
+    editor.redo();
+    assert!(
+        !editor.changed(),
+        "Redoing to save point should set editor as unchanged"
+    );
+}
+
+#[test]
+fn redoing_past_save_point_sets_editor_to_changed() {
+    let mut editor = editor();
+
+    // Save point change to undo to and then redo past.
+    editor.start_change();
+    editor.insert_string("Walt Whitman ", None);
+    editor.finish_change();
+
+    // Set save point to the change above.
+    assert!(
+        editor.changed(),
+        "Editor should be set as modified after insert() and finish_change()"
+    );
+    editor.save_point();
+    assert!(
+        !editor.changed(),
+        "Editor should be unchanged after setting a save point"
+    );
+
+    editor.start_change();
+    editor.insert_string("Allen Ginsberg ", None);
+    editor.finish_change();
+
+    editor.start_change();
+    editor.insert_string("Jack Kerouac ", None);
+    editor.finish_change();
+
+    assert!(editor.changed(), "Editor should be modified insertion");
+
+    // Undo to Whitman
+    editor.undo();
+    editor.undo();
+    assert!(
+        !editor.changed(),
+        "Editor should be unmodified after undoing to the save point"
+    );
+
+    // Redo to Kerouac
+    editor.redo();
+    editor.redo();
+    assert!(
+        editor.changed(),
+        "Editor should be modified after redoing past the save point"
+    );
+}
+
+#[test]
+fn undoing_past_save_point_sets_editor_to_changed() {
+    let mut editor = editor();
+
+    editor.start_change();
+    editor.insert_string("Robert Fripp ", None);
+    editor.finish_change();
+
+    // Save point change to undo past.
+    editor.start_change();
+    editor.insert_string("Thurston Moore ", None);
+    editor.finish_change();
+
+    assert!(editor.changed(), "Editor should be changed after insertion");
+    editor.save_point();
+    assert!(
+        !editor.changed(),
+        "Editor should be unchanged after setting a save point"
+    );
+
+    editor.start_change();
+    editor.insert_string("Kim Deal ", None);
+    editor.finish_change();
+
+    // Undo to the first change
+    editor.undo();
+    editor.undo();
+    assert!(
+        editor.changed(),
+        "Editor should be changed after undoing past save point"
+    );
+}
+
+// #[test]
+// fn undo_all_changes() {
+//     unimplemented!()
+// }