From 96172e13a7a316a5fc038b61aa836d3f91d8a837 Mon Sep 17 00:00:00 2001 From: clover caruso Date: Sat, 10 Oct 2026 21:32:55 -0700 Subject: [PATCH] fix: keep Undo and Redo history intact when recycling is busy Carry the removed action position through preflight and restore the original action to its original stack when structural ownership is unavailable. A retry must remain behind newer edits on other pages. Restoration happens only before work starts; uncertain asynchronous publication is not replayed. Cover contention and exact history ordering for both Undo and Redo. Fixes #119 Assisted-by: gpt-6.1-sol --- crates/snowbound/src/undo.rs | 109 +++++++++++++++++++++++++++-------- 1 file changed, 86 insertions(+), 23 deletions(-) diff --git a/crates/snowbound/src/undo.rs b/crates/snowbound/src/undo.rs index 39172b1b05ad348bb27d7ef4f7685f8759d8ccc4..2c5b5e82b7e185e049a38c733304ff660980b370 100644 --- a/crates/snowbound/src/undo.rs +++ b/crates/snowbound/src/undo.rs @@ -145,7 +145,8 @@ impl Action { pub enum Next { /// The open page's last edit, or the last undone. Edit, - Action(Action), + /// An action and its original position until preflight accepts it. + Action(Action, usize), /// The page a created page Undo would take out still holds edits made since, so /// Undo shows it, where those go back first. Show(ExGuid), @@ -168,6 +169,12 @@ impl Timeline { } } + /// Restores a refused action before newer edits on other pages, in its original stack. + fn put_back(&mut self, (redo, at): (bool, usize), action: Action) { + let steps = if redo { &mut self.redo } else { &mut self.undo }; + steps.insert(at, Step::Action(action)); + } + /// Follows page `page`'s history, `depth` steps of Undo and Redo, after an edit or /// before one is taken: steps it gained are the newest; `run`, a run of typing that /// went on, makes its last step the newest. @@ -232,7 +239,7 @@ impl Timeline { Step::Action(_) => {} } match steps.remove(at) { - Step::Action(action) => Some(Next::Action(action)), + Step::Action(action) => Some(Next::Action(action, at)), Step::Edit(_) => unreachable!("an edit is not taken off"), } } @@ -988,7 +995,7 @@ impl State { } } Some(Next::Show(page)) => self.commands.push(Command::OpenPage(page)), - Some(Next::Action(action)) => self.perform(action, Some(redo))?, + Some(Next::Action(action, at)) => self.perform(action, Some((redo, at)))?, } Ok(()) } @@ -998,7 +1005,7 @@ impl State { pub(crate) fn perform( &mut self, action: Action, - taken: Option, + taken: Option<(bool, usize)>, ) -> Result<(), Box> { let Action { notebook, @@ -1014,7 +1021,7 @@ impl State { .ok_or("That notebook is closed")?; if !holds(&library.themes(), &from) { return match taken { - Some(redo) => self.take(redo), + Some((redo, _)) => self.take(redo), None => Ok(()), }; } @@ -1038,7 +1045,7 @@ impl State { section, change: Change::Themes { from: to, to: from }, }; - self.undo.note(taken, undo); + self.undo.note(taken.map(|(redo, _)| redo), undo); return self.wear_theme(true); } let Some(section) = section else { @@ -1050,11 +1057,11 @@ impl State { .ok_or("That notebook is closed")?; let Some((structure, undo)) = restructuring(&library, &change) else { return match taken { - Some(redo) => self.take(redo), + Some((redo, _)) => self.take(redo), None => Ok(()), }; }; - self.undo.note(taken, undo); + self.undo.note(taken.map(|(redo, _)| redo), undo); self.restructure(library, structure); return Ok(()); }; @@ -1065,13 +1072,20 @@ impl State { Change::Delete { created: false, .. } | Change::Restore { created: false, .. } ) && matches!(session.library.notebook, Ok(Some(_))) { - Some( - session - .library - .structure - .try_lock_arc() - .ok_or_else(|| std::io::Error::from(std::io::ErrorKind::WouldBlock))?, - ) + let Some(changing) = session.library.structure.try_lock_arc() else { + if let Some(taken) = taken { + self.undo.put_back( + taken, + Action { + notebook, + section: Some(section), + change, + }, + ); + } + return Err(std::io::Error::from(std::io::ErrorKind::WouldBlock).into()); + }; + Some(changing) } else { None }; @@ -1125,7 +1139,7 @@ impl State { let Some(applied) = applied else { // What it would change is gone: Undo goes on to the step before. return match taken { - Some(redo) if stale => { + Some((redo, _)) if stale => { state.undo.waiting.push_front(redo); Ok(()) } @@ -1138,7 +1152,7 @@ impl State { section: Some(section), change: applied.undo, }; - state.undo.note(taken, undo); + state.undo.note(taken.map(|(redo, _)| redo), undo); state.edited(Vec::new()); Ok(()) }))); @@ -1207,7 +1221,7 @@ mod tests { } fn is_action(next: Option) -> bool { - matches!(next, Some(Next::Action(_))) + matches!(next, Some(Next::Action(..))) } fn is_edit(next: Option) -> bool { @@ -1288,10 +1302,13 @@ mod tests { assert!(!timeline.reaches(false, &other)); assert!(matches!( timeline.next(false, page(7), [0, 0], &elsewhere), - Some(Next::Action(Action { - change: Change::Rename { .. }, - .. - })) + Some(Next::Action( + Action { + change: Change::Rename { .. }, + .. + }, + _ + )) )); assert!(timeline.next(false, page(7), [0, 0], &elsewhere).is_none()); assert!(is_action(timeline.next(false, page(7), [0, 0], &here()))); @@ -1306,7 +1323,7 @@ mod tests { let mut timeline = Timeline::default(); let shown = page(1); timeline.record(created(page(2))); - let Some(Next::Action(action)) = timeline.next(false, shown, [0, 0], &here()) else { + let Some(Next::Action(action, _)) = timeline.next(false, shown, [0, 0], &here()) else { panic!() }; timeline.note(Some(false), action); @@ -1320,6 +1337,52 @@ mod tests { assert!(!timeline.reaches(true, &here())); } + #[test] + fn contended_page_actions_keep_their_position_in_undo_and_redo() { + for redo in [false, true] { + let mut timeline = Timeline::default(); + let action = Action { + change: Change::Delete { + pages: vec![page(2)], + show: None, + created: false, + }, + ..created(page(2)) + }; + let steps = if redo { + &mut timeline.redo + } else { + &mut timeline.undo + }; + steps.push(Step::Action(action)); + steps.push(Step::Edit(page(3))); + let Some(Next::Action(action, at)) = timeline.next(redo, page(1), [0, 0], &here()) + else { + panic!("the current page has a pending action"); + }; + let structure = Arc::new(async_lock::Mutex::new(())); + let changing = structure.try_lock_arc().unwrap(); + assert!(structure.try_lock_arc().is_none()); + timeline.put_back((redo, at), action); + let depth = if redo { [0, 1] } else { [1, 0] }; + assert!( + is_edit(timeline.next(redo, page(3), depth, &here())), + "newer edit stays first" + ); + timeline.stepped(page(3), redo); + drop(changing); + let _changing = structure.try_lock_arc().unwrap(); + let Some(Next::Action(action, at)) = timeline.next(redo, page(1), [0, 0], &here()) + else { + panic!("the refused action must remain in its original stack"); + }; + assert_eq!(at, 0); + assert!( + matches!(action.change, Change::Delete { pages, created: false, .. } if pages == [page(2)]) + ); + } + } + const AUTHOR: &str = "Rust Author"; fn dated() -> PageCreation { -- 2.54.0