From 577710e1d47638bbd086a22366de263d858f4475 Mon Sep 17 00:00:00 2001 From: clover caruso Date: Fri, 2 Oct 2026 12:52:24 -0700 Subject: [PATCH] fix: Undo and Redo pressed while a page or action is on its way wait for it Presses during an in-flight page action or page switch were dropped (the command was disabled while busy), so quick Undo presses were lost and the replay test undo_walks_back_through_new_pages_and_their_titles flaked under load. They now queue and are taken in order once nothing is on its way. An action that shows a page counts as switching until it lands. Replay gains `settle`, and `accessibility` waits for the same settled state before writing the tree, so replay tests stop racing background work. Assisted-by: claude-opus-5.5 --- crates/snowbound/src/main.rs | 48 ++++++++++++++++----- crates/snowbound/src/undo.rs | 72 +++++++++++++++++++++----------- crates/snowbound/tests/replay.rs | 2 +- 3 files changed, 87 insertions(+), 35 deletions(-) diff --git a/crates/snowbound/src/main.rs b/crates/snowbound/src/main.rs index acdf9b164c8b79803770cb0b23cd9801e9199fcc..ffb1f9a43de6ed908ec47194764a56a53fc44d17 100644 --- a/crates/snowbound/src/main.rs +++ b/crates/snowbound/src/main.rs @@ -363,8 +363,9 @@ enum Replay { Pinch(f32), /// Paints the next frame into a PNG as well as the window. Snapshot(PathBuf), - /// Writes the window's accessibility tree as text. - Accessibility(PathBuf), + /// Waits for nothing to be on its way, then writes the window's accessibility tree as + /// text where given, and answers. + Settle(Option, std::sync::mpsc::Sender<()>), /// A frame during a wait, as a visible window's display would ask for. Tick, Appearance(winit::window::Theme), @@ -883,6 +884,9 @@ struct State { strip_held: bool, /// Where a replay asked the next frame to be written. snapshot: Option, + /// A replay waiting for nothing to be on its way, and where it wants the accessibility + /// tree written then. + replay_settle: Option<(Option, std::sync::mpsc::Sender<()>)>, /// `SNOWBOUND_FRAMES`: a directory every frame drawn is also written to, named by /// milliseconds since the window opened. frames: Option<(PathBuf, Instant)>, @@ -1226,6 +1230,7 @@ impl State { strip_press: None, strip_held: false, snapshot: None, + replay_settle: None, frames: std::env::var_os("SNOWBOUND_FRAMES").map(|dir| (dir.into(), Instant::now())), initial, initial_date, @@ -1286,6 +1291,9 @@ impl State { if let Err(error) = self.open_loaded() { eprintln!("{error}"); } + if let Err(error) = self.take_waiting() { + eprintln!("{error}"); + } lap("open", start); self.follow_reading(); let size = self.window.inner_size(); @@ -5706,11 +5714,7 @@ impl State { } } Replay::Snapshot(path) => self.snapshot = Some(path), - Replay::Accessibility(path) => { - if let Err(error) = self.write_accessibility(&path) { - eprintln!("{error}"); - } - } + Replay::Settle(path, settled) => self.replay_settle = Some((path, settled)), Replay::Tick | Replay::Quit => {} Replay::Appearance(appearance) => { self.window.set_theme(Some(appearance)); @@ -5726,6 +5730,16 @@ impl State { if let Err(error) = self.frame() { eprintln!("{error}"); } + if self.settled() + && let Some((path, settled)) = self.replay_settle.take() + { + if let Some(path) = path + && let Err(error) = self.write_accessibility(&path) + { + eprintln!("{error}"); + } + let _ = settled.send(()); + } } #[cfg(not(target_arch = "wasm32"))] UserEvent::Accessibility(event) => self.access_event(event), @@ -6041,10 +6055,12 @@ fn write_png(path: &Path, size: [u32; 2], pixels: &[u8]) -> Result<(), Box) -> Result<(), Box> { + let (settle, settled) = std::sync::mpsc::channel(); let mut steps = Vec::new(); for line in script.lines().filter(|line| !line.trim().is_empty()) { let (command, rest) = line.split_once(' ').unwrap_or((line, "")); @@ -6113,7 +6129,8 @@ fn replay(script: String, proxy: EventLoopProxy) -> Result<(), Box Ok(Replay::Snapshot(rest.into())), - "accessibility" => Ok(Replay::Accessibility(rest.into())), + "settle" => Ok(Replay::Settle(None, settle.clone())), + "accessibility" => Ok(Replay::Settle(Some(rest.into()), settle.clone())), "appearance" => Ok(Replay::Appearance(match rest { "light" => winit::window::Theme::Light, "dark" => winit::window::Theme::Dark, @@ -6129,7 +6146,18 @@ fn replay(script: String, proxy: EventLoopProxy) -> Result<(), Box { // Ticks keep a 60 Hz display's pace, dropping those a slow frame missed. diff --git a/crates/snowbound/src/undo.rs b/crates/snowbound/src/undo.rs index 04b842ee04abd1ffe30633d26d369262932a03ed..1fd73ba1ce755ea367e080ea153c1ce79c7a742d 100644 --- a/crates/snowbound/src/undo.rs +++ b/crates/snowbound/src/undo.rs @@ -23,8 +23,10 @@ pub struct Timeline { redo: Vec, /// Editors of pages left holding history, the longest left first. parked: Vec<(ExGuid, CanvasEditor)>, - /// Actions on their way, while which Undo and Redo wait. + /// Actions on their way. busy: usize, + /// Presses of Undo, or Redo when true, that wait for an action or page on its way. + waiting: std::collections::VecDeque, } enum Step { @@ -185,9 +187,6 @@ impl Timeline { depth: [usize; 2], here: &Here, ) -> Option { - if self.busy > 0 { - return None; - } self.edited(page, depth, false); let steps = if redo { &mut self.redo } else { &mut self.undo }; let at = steps.iter().rposition(|step| match step { @@ -235,10 +234,9 @@ impl Timeline { /// Whether Undo, or `redo` Redo, has an action to take `here`. pub fn reaches(&self, redo: bool, here: &Here) -> bool { let steps = if redo { &self.redo } else { &self.undo }; - self.busy == 0 - && steps - .iter() - .any(|step| matches!(step, Step::Action(action) if action.applies(here))) + steps + .iter() + .any(|step| matches!(step, Step::Action(action) if action.applies(here))) } /// Keeps the editor of page `page`, just left, for its history. @@ -815,21 +813,47 @@ impl State { }) } - /// Whether Undo, or `redo` Redo, has something to take. + /// Whether no action, page or command is on its way, which Undo and Redo wait for. + pub(crate) fn settled(&self) -> bool { + self.undo.busy == 0 && self.switching.is_none() && self.commands.is_empty() + } + + /// Whether Undo, or `redo` Redo, has something to take, or may once what is on its way + /// lands. pub(crate) fn can_step(&self, redo: bool) -> bool { let editor = &self.view.editor; - (if redo { - editor.can_redo() - } else { - editor.can_undo() - }) || self - .here() - .is_some_and(|here| self.undo.reaches(redo, &here)) + !self.settled() + || (if redo { + editor.can_redo() + } else { + editor.can_undo() + }) + || self + .here() + .is_some_and(|here| self.undo.reaches(redo, &here)) + } + + /// Undo, or `redo` Redo, once what is on its way lands. + pub(crate) fn step(&mut self, redo: bool) -> Result<(), Box> { + self.undo.waiting.push_back(redo); + self.take_waiting() + } + + /// Takes the Undo and Redo presses waiting, while nothing is on its way. + pub(crate) fn take_waiting(&mut self) -> Result<(), Box> { + while self.settled() + && let Some(redo) = self.undo.waiting.pop_front() + { + if self.can_step(redo) { + self.take(redo)?; + } + } + Ok(()) } /// Undo, or `redo` Redo: the open page's last edit or the last action made where the /// user is, whichever came last. - pub(crate) fn step(&mut self, redo: bool) -> Result<(), Box> { + fn take(&mut self, redo: bool) -> Result<(), Box> { self.persist()?; let depth = self.view.editor.history_depth(); // A page kept in no notebook has only its own history. @@ -876,7 +900,7 @@ impl State { .ok_or("That notebook is closed")?; let Some((structure, undo)) = restructuring(&library, &change) else { return match taken { - Some(redo) => self.step(redo), + Some(redo) => self.take(redo), None => Ok(()), }; }; @@ -895,6 +919,7 @@ impl State { let shown = session.space; let proxy = self.proxy.clone(); self.undo.busy += 1; + self.switching = Some((None, web_time::Instant::now())); self.load(move || { let applied = site.change(change, shown); let show = match &applied { @@ -908,7 +933,10 @@ 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 => state.step(redo), + Some(redo) if stale => { + state.undo.waiting.push_front(redo); + Ok(()) + } _ => Ok(()), }; }; @@ -1079,8 +1107,7 @@ mod tests { assert!(!timeline.reaches(false, &here())); } - /// Redo takes back the last undone first; a new edit ends what actions it held, and - /// waits while an action is on its way. + /// Redo takes back the last undone first; a new edit ends what actions it held. #[test] fn redo_mirrors_undo_and_new_edits_end_it() { let mut timeline = Timeline::default(); @@ -1098,9 +1125,6 @@ mod tests { timeline.stepped(shown, true); timeline.stepped(shown, false); assert!(!timeline.reaches(true, &here())); - timeline.record(created(page(3))); - timeline.busy = 1; - assert!(timeline.next(false, shown, [0, 1], &here()).is_none()); } const AUTHOR: &str = "Rust Author"; diff --git a/crates/snowbound/tests/replay.rs b/crates/snowbound/tests/replay.rs index b867e7bb24f1d887ff62a3b0771c3ac9c15601d3..e9cb503f650da9a7c1416b594c2c7518d02fa61e 100644 --- a/crates/snowbound/tests/replay.rs +++ b/crates/snowbound/tests/replay.rs @@ -205,7 +205,7 @@ fn undo_walks_back_through_new_pages_and_their_titles() { let mut steps = Vec::new(); for title in ["type One", "type Two"] { steps.extend(chord("modifiers command", "key n")); - steps.extend([title, "wait 300"].map(String::from)); + steps.extend(["settle", title, "wait 300"].map(String::from)); } steps.extend(chord("modifiers command control", "key Left")); steps.push("accessibility back".into()); -- 2.54.0