diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index fde9638..5890493 100644 --- a/crates/ui-egui/src/app.rs +++ b/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, + /// What each Undo took back, newest last, so Redo puts it back the same way. + undone: Vec, /// 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. diff --git a/crates/ui-egui/src/inkpad.rs b/crates/ui-egui/src/inkpad.rs index b96cbe9..b91f81f 100644 --- a/crates/ui-egui/src/inkpad.rs +++ b/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, undo_steps: Vec<(Vec, 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>, /// 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()); diff --git a/crates/ui-egui/tests/ui.rs b/crates/ui-egui/tests/ui.rs index cbc7687..a92b1fe 100644 --- a/crates/ui-egui/tests/ui.rs +++ b/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,