fix(ui-egui): ink undo starts at the opened page, and can be redone (#155, #156)

crates/ui-egui/src/app.rs

@@ -549,6 +549,14 @@ fn page_title(ui: &mut Ui, ed: &mut Editor, tokens: &Tokens, title_id: egui::Id,
 }
 
 /// What to do with a file the user dropped on a page.
+/// What one Undo took back: the drawing, the page's words and boxes, or both at once.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+enum Undone {
+    Ink,
+    Page,
+    Both,
+}
+
 #[derive(Clone, Copy, PartialEq, Eq)]
 enum FileChoice {
     /// Copy it into the page's folder, so the notebook carries it. OneNote: "Attach File".
@@ -705,6 +713,8 @@ pub struct NoteCraftApp {
     qat: quickaccess::Config,
     /// When the pen last changed the page, to tell which Undo should take back.
     last_ink_change: Option<Instant>,
+    /// What each Undo took back, newest last, so Redo puts it back the same way.
+    undone: Vec<Undone>,
     /// Two columns in the side panel (organisers, then pages) instead of one tree.
     split_panes: bool,
     /// Whether the organisers column is showing, which the three-bar button toggles.
@@ -857,6 +867,7 @@ impl NoteCraftApp {
             nav: NavHistory::default(),
             qat,
             last_ink_change: None,
+            undone: Vec::new(),
             open: HashSet::new(),
             selected: None,
             split_panes: session_split,
@@ -1188,11 +1199,11 @@ impl NoteCraftApp {
     }
 
     fn can_undo(&self) -> bool {
-        self.editor.as_ref().is_some_and(|e| e.writable && e.history.can_undo()) || self.pad.as_ref().is_some_and(|p| p.strokes() > 0)
+        self.editor.as_ref().is_some_and(|e| e.writable && e.history.can_undo()) || self.pad.as_ref().is_some_and(crate::inkpad::Pad::can_undo)
     }
 
     fn can_redo(&self) -> bool {
-        self.editor.as_ref().is_some_and(|e| e.writable && e.history.can_redo())
+        self.editor.as_ref().is_some_and(|e| e.writable && e.history.can_redo()) || self.pad.as_ref().is_some_and(crate::inkpad::Pad::can_redo)
     }
 
     /// Undo for the Quick Access Toolbar: the page's words and boxes (the page history in
@@ -1211,18 +1222,42 @@ impl NoteCraftApp {
         let together = matches!((inked, edited), (Some(i), Some(e)) if i.max(e).duration_since(i.min(e)) < Duration::from_millis(300));
         let ink_can = self.pad.as_ref().is_some_and(crate::inkpad::Pad::can_undo);
         let page_can = self.editor.as_ref().is_some_and(|e| e.writable && e.history.can_undo());
-        if ink_can && page_can && together {
+        let what = if ink_can && page_can && together {
             self.ink_undo();
             self.step_back(true);
+            Undone::Both
         } else if ink_can && (ink_newer || !page_can) {
             self.ink_undo();
+            Undone::Ink
         } else {
             self.step_back(true);
-        }
+            Undone::Page
+        };
+        self.undone.push(what);
     }
 
+    /// Puts back what Undo last took back, from the ink, the page or both, the way it was taken
+    /// (issue #155). When the record of which no longer fits what either can put back, the page
+    /// goes first, then the ink.
     fn redo(&mut self) {
-        self.step_back(false);
+        let ink_can = self.pad.as_ref().is_some_and(crate::inkpad::Pad::can_redo);
+        let page_can = self.editor.as_ref().is_some_and(|e| e.writable && e.history.can_redo());
+        match self.undone.pop() {
+            Some(Undone::Both) if ink_can && page_can => {
+                self.ink_redo();
+                self.step_back(false);
+            }
+            Some(Undone::Ink) if ink_can => self.ink_redo(),
+            Some(Undone::Page) if page_can => self.step_back(false),
+            _ if !page_can && ink_can => {
+                self.undone.clear();
+                self.ink_redo();
+            }
+            _ => {
+                self.undone.clear();
+                self.step_back(false);
+            }
+        }
     }
 
     /// The keys and buttons that move through the pages that were opened. Undo and Redo keys
@@ -1542,6 +1577,8 @@ impl NoteCraftApp {
                 log::debug!("attachment recovery scan found no missing icons: page={:?} attachments={}", dir, attachments.len());
             }
             self.pad = Some(crate::inkpad::Pad::of_page(dir));
+            // Another page, another history: nothing undone on the last one is redone here.
+            self.undone.clear();
             self.absorb_old_sheets(dir);
             self.sync_ink_reference();
             self.selected = Some(dir.to_path_buf());
@@ -3063,8 +3100,8 @@ impl NoteCraftApp {
             table_borders: self.table_borders,
             gridcraft: self.gridcraft.is_some(),
             page_colour: self.editor.as_ref().and_then(|e| e.colour),
-            can_undo: self.editor.as_ref().is_some_and(|e| e.history.can_undo()),
-            can_redo: self.editor.as_ref().is_some_and(|e| e.history.can_redo()),
+            can_undo: self.editor.as_ref().is_some_and(|e| e.history.can_undo()) || self.pad.as_ref().is_some_and(crate::inkpad::Pad::can_undo),
+            can_redo: self.editor.as_ref().is_some_and(|e| e.history.can_redo()) || self.pad.as_ref().is_some_and(crate::inkpad::Pad::can_redo),
             sensitivity: self.sensitivity,
             pressure_available: self.stylus.sample().pen,
             has_page: self.editor.is_some(),
@@ -3740,6 +3777,24 @@ impl NoteCraftApp {
         }
     }
 
+    /// Puts back the change to the drawing that undo last took back.
+    fn ink_redo(&mut self) {
+        let Some(pad) = self.pad.as_mut() else {
+            self.say(crate::tl_id!("ui.nothing_to_redo"));
+            return;
+        };
+        if !pad.redo() {
+            self.say(crate::tl_id!("ui.nothing_to_redo"));
+            return;
+        }
+        let strokes = pad.strokes();
+        match pad.save() {
+            Ok(()) => self.say(crate::i18n::format_message_id("ui.drawing_saved_count_strokes", &[("count", &strokes.to_string())])),
+            Err(e) => self.complain(&crate::i18n::format_message_id("ui.cannot_write_the_drawing_error", &[("error", &e.to_string())])),
+        }
+        self.sync_ink_reference();
+    }
+
     /// Takes the last stroke back out of the drawing on this page.
     fn ink_undo(&mut self) {
         let Some(pad) = self.pad.as_mut() else {
@@ -7343,7 +7398,8 @@ impl NoteCraftApp {
         // one that answers, or Ctrl+Z would mean two different things on one page.
         let ctrl_shift = Modifiers { ctrl: true, shift: true, command: true, ..Modifiers::NONE };
         if ctx.input_mut(|i| i.consume_key(ctrl_shift, Key::Z) || i.consume_key(Modifiers::COMMAND, Key::Y)) {
-            self.step_back(false);
+            // The ink's as well as the page's, whichever Undo took back.
+            self.redo();
         } else if ctx.input_mut(|i| i.consume_key(Modifiers::COMMAND, Key::Z)) {
             // The ink's changes as well as the page's: whichever was last, or both when one
             // press made them together.

crates/ui-egui/src/inkpad.rs

@@ -103,6 +103,9 @@ pub struct Pad {
     /// stroke drawn, rubbed out, deleted, moved or stretched), undo puts back what was there.
     kept: Vec<Stroke>,
     undo_steps: Vec<(Vec<Stroke>, std::time::Instant)>,
+    /// The strokes as they were before each undo, newest last, for redo to put back. A new
+    /// change starts a new branch and empties it (issue #155).
+    redo_steps: Vec<Vec<Stroke>>,
     /// Changed since the last finished change was kept.
     touched: bool,
 }
@@ -140,6 +143,7 @@ impl Pad {
             shift: (0.0, 0.0),
             kept: Vec::new(),
             undo_steps: Vec::new(),
+            redo_steps: Vec::new(),
             touched: false,
         }
         .with_kept()
@@ -195,22 +199,34 @@ impl Pad {
         came
     }
 
-    /// Takes the last stroke back.
-    /// Takes the last change to the ink back, whatever it was. With nothing kept to go back to
-    /// (ink drawn before the page was opened), the last stroke goes, as it always did.
+    /// Takes the last change to the ink back, whatever it was, and keeps it for redo. Only a
+    /// change made since the page was opened is taken back: ink that was already on the page
+    /// is where the page starts, not a change, and undo used to delete it (issue #156).
     pub fn undo(&mut self) -> bool {
         self.let_go();
         self.settle();
-        let went = match self.undo_steps.pop() {
-            Some((before, _)) => {
-                self.drawing.strokes = before;
-                true
-            }
-            None => self.drawing.undo(),
-        };
+        let Some((before, _)) = self.undo_steps.pop() else { return false };
+        self.redo_steps.push(std::mem::replace(&mut self.drawing.strokes, before));
         self.kept = self.drawing.strokes.clone();
-        self.dirty |= went;
-        went
+        self.dirty = true;
+        true
+    }
+
+    /// Puts back the change undo last took back, while nothing new has been done since.
+    pub fn redo(&mut self) -> bool {
+        self.let_go();
+        self.settle();
+        let Some(after) = self.redo_steps.pop() else { return false };
+        let before = std::mem::replace(&mut self.drawing.strokes, after);
+        self.undo_steps.push((before, std::time::Instant::now()));
+        self.kept = self.drawing.strokes.clone();
+        self.dirty = true;
+        true
+    }
+
+    /// Whether redo has anything to put back.
+    pub fn can_redo(&self) -> bool {
+        !self.redo_steps.is_empty()
     }
 
     pub fn strokes(&self) -> usize {
@@ -251,6 +267,7 @@ impl Pad {
         }
         let before = std::mem::replace(&mut self.kept, self.drawing.strokes.clone());
         self.undo_steps.push((before, std::time::Instant::now()));
+        self.redo_steps.clear();
         if self.undo_steps.len() > INK_UNDO_STEPS {
             self.undo_steps.remove(0);
         }
@@ -261,9 +278,9 @@ impl Pad {
         self.undo_steps.last().map(|(_, when)| *when)
     }
 
-    /// Whether undo has anything to take back.
+    /// Whether undo has anything to take back: a change made since the page was opened.
     pub fn can_undo(&self) -> bool {
-        !self.undo_steps.is_empty() || !self.drawing.strokes.is_empty()
+        !self.undo_steps.is_empty()
     }
 
     /// Picks up every stroke a selection box touches; `rect` is two corners in page points.
@@ -871,12 +888,68 @@ mod tests {
         assert_eq!(reloaded.drawing.strokes[0].dabs[1].y, 1240.0);
     }
 
+    /// A stroke added as the pen adds one: a change, which undo can take back once it settles.
+    fn draw(pad: &mut Pad, s: Stroke) {
+        assert!(pad.drawing.add(s));
+        pad.touched = true;
+        pad.dirty = true;
+        pad.settle();
+    }
+
+    /// Issue #156: ink that was on the page when it was opened is where the page starts. Undo
+    /// with nothing done since has nothing to take back, and leaves it alone.
+    #[test]
+    fn undo_after_opening_a_page_leaves_its_ink_alone() {
+        let t = TempDir::new("ink-undo-baseline");
+        let page = t.path().to_path_buf();
+        let mut first = Pad::of_page(&page);
+        draw(&mut first, stroke(10.0));
+        first.save().expect("saved");
+
+        let mut reopened = Pad::of_page(&page);
+        assert_eq!(reopened.strokes(), 1);
+        assert!(!reopened.can_undo(), "nothing has been done since the page was opened");
+        assert!(!reopened.undo());
+        assert_eq!(reopened.strokes(), 1, "the saved stroke is still there");
+
+        draw(&mut reopened, stroke(30.0));
+        assert!(reopened.undo(), "a stroke drawn now can be taken back");
+        assert_eq!(reopened.strokes(), 1, "back to the stroke the page was opened with, and no further");
+        assert!(!reopened.undo());
+    }
+
+    /// Issue #155: what undo took back, redo puts back, until something new is done.
+    #[test]
+    fn redo_puts_back_what_undo_took() {
+        let t = TempDir::new("ink-redo");
+        let mut pad = Pad::of_page(t.path());
+        draw(&mut pad, stroke(10.0));
+        draw(&mut pad, stroke(20.0));
+        assert!(!pad.can_redo());
+
+        assert!(pad.undo() && pad.undo());
+        assert_eq!(pad.strokes(), 0);
+        assert!(pad.can_redo());
+        assert!(pad.redo());
+        assert_eq!(pad.strokes(), 1);
+        assert!(pad.redo());
+        assert_eq!(pad.strokes(), 2, "both strokes are back, in order");
+        assert!(!pad.redo(), "and nothing more");
+        assert!(pad.undo(), "what redo put back can be taken back again");
+        assert_eq!(pad.strokes(), 1);
+
+        draw(&mut pad, stroke(40.0));
+        assert!(!pad.can_redo(), "a new stroke starts a new branch");
+        assert!(!pad.redo());
+        assert_eq!(pad.strokes(), 2);
+    }
+
     #[test]
     fn rubbing_a_page_clean_takes_the_file_away_again() {
         let t = TempDir::new("ink-layer-clean");
         let page = t.path().to_path_buf();
         let mut pad = Pad::of_page(&page);
-        pad.drawing.add(stroke(10.0));
+        draw(&mut pad, stroke(10.0));
         pad.save().expect("saved");
         assert!(page.join(FILE).exists());
 

crates/ui-egui/tests/ui.rs

@@ -2114,6 +2114,59 @@ fn ink_comes_back_when_the_page_is_opened_again() {
     assert!(!std::fs::read_to_string(&plain).unwrap_or_default().contains("nc:"), "and holds no readings");
 }
 
+/// Issues #155 and #156: Redo puts back a stroke Undo took, and Undo on a page just opened
+/// does not take away the ink it was opened with.
+#[test]
+fn undo_and_redo_of_ink_follow_what_was_done_this_session() {
+    let (_t, mut app, s) = sample_app("ui-ink-undo-redo");
+    app.open_path(&s.html_page.to_string_lossy());
+    let mut h = harness(app, LIGHT);
+    settle(&mut h);
+    h.get_by_label("Draw").click();
+    pump(&mut h);
+    h.get_by_label("Pen red").click();
+    pump(&mut h);
+    let info = ask(&mut h, "ui.inspect", json!({})).expect("inspect");
+    let origin = info.pointer("/page/origin").and_then(Value::as_array).expect("page origin");
+    let (ox, oy) = (origin[0].as_f64().unwrap_or(0.0), origin[1].as_f64().unwrap_or(0.0));
+    let y = oy + 160.0;
+    ask(&mut h, "ui.press", json!({"x": ox + 60.0, "y": y})).expect("press");
+    pump(&mut h);
+    for x in [ox + 120.0, ox + 180.0, ox + 240.0] {
+        ask(&mut h, "ui.move", json!({"x": x, "y": y})).expect("draw");
+        pump(&mut h);
+    }
+    ask(&mut h, "ui.release", json!({"x": ox + 240.0, "y": y})).expect("release");
+    settle(&mut h);
+    let strokes = |h: &mut Harness<'_, NoteCraftApp>| ask(h, "ui.inspect", json!({})).expect("inspect")["ink"]["strokes"].as_u64();
+    assert_eq!(strokes(&mut h), Some(1), "one stroke drawn");
+
+    ask(&mut h, "ui.undo", json!({})).expect("undo");
+    settle(&mut h);
+    assert_eq!(strokes(&mut h), Some(0), "undo took it back");
+    ask(&mut h, "ui.redo", json!({})).expect("redo");
+    settle(&mut h);
+    assert_eq!(strokes(&mut h), Some(1), "redo put it back");
+    // The keys do the same as the buttons: Ctrl+Z takes the stroke back, Ctrl+Y puts it back.
+    ask(&mut h, "ui.key", json!({"key": "Z", "ctrl": true})).expect("Ctrl+Z");
+    settle(&mut h);
+    assert_eq!(strokes(&mut h), Some(0), "Ctrl+Z took it back");
+    ask(&mut h, "ui.key", json!({"key": "Y", "ctrl": true})).expect("Ctrl+Y");
+    settle(&mut h);
+    assert_eq!(strokes(&mut h), Some(1), "Ctrl+Y put it back");
+    assert_ne!(h.state().status(), "Nothing to redo");
+
+    // Away and back: the stroke is on the page it was saved with, and nothing was done since.
+    ask(&mut h, "ui.open", json!({"path": &s.md_page.to_string_lossy()})).expect("another page");
+    settle(&mut h);
+    ask(&mut h, "ui.open", json!({"path": &s.html_page.to_string_lossy()})).expect("back");
+    settle(&mut h);
+    assert_eq!(strokes(&mut h), Some(1));
+    ask(&mut h, "ui.undo", json!({})).expect("undo with nothing done");
+    settle(&mut h);
+    assert_eq!(strokes(&mut h), Some(1), "the saved stroke is not taken away");
+}
+
 /// Every tool on the rack can be picked up. This is the test that was missing when a
 /// second interact was laid over each pen for its right-click menu: later widgets win in
 /// egui, so the left click was swallowed and only the eraser, which had no such overlay,