Fix: Undo and redo correctly updates editor modified status

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!()
+// }