authorgravatar for git@paperclover.netclover caruso <git@paperclover.net> 2026-10-02 12:52:24-07:00
committergravatar for git@paperclover.netclover caruso <git@paperclover.net> 2026-10-02 13:41:33-07:00
log577710e1d47638bbd086a22366de263d858f4475
tree2c03317dcef272b0ef7c986aab8f918f9c13ff4f
parent30964a9cc80080214547f04dcb744f23406dffd6
signature Signed by SSH key SHA256:52mNGHRsVFBDED9IAX5pe+LRWUefqTbxEReunq21QvU

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

3 files changed, 87 insertions(+), 35 deletions(-)

crates/snowbound/src/main.rs+38-10
......@@ -363,8 +363,9 @@ enum Replay {
363363 Pinch(f32),
364364 /// Paints the next frame into a PNG as well as the window.
365365 Snapshot(PathBuf),
366 /// Writes the window's accessibility tree as text.
367 Accessibility(PathBuf),
366 /// Waits for nothing to be on its way, then writes the window's accessibility tree as
367 /// text where given, and answers.
368 Settle(Option<PathBuf>, std::sync::mpsc::Sender<()>),
368369 /// A frame during a wait, as a visible window's display would ask for.
369370 Tick,
370371 Appearance(winit::window::Theme),
......@@ -883,6 +884,9 @@ struct State {
883884 strip_held: bool,
884885 /// Where a replay asked the next frame to be written.
885886 snapshot: Option<PathBuf>,
887 /// A replay waiting for nothing to be on its way, and where it wants the accessibility
888 /// tree written then.
889 replay_settle: Option<(Option<PathBuf>, std::sync::mpsc::Sender<()>)>,
886890 /// `SNOWBOUND_FRAMES`: a directory every frame drawn is also written to, named by
887891 /// milliseconds since the window opened.
888892 frames: Option<(PathBuf, Instant)>,
......@@ -1226,6 +1230,7 @@ impl State {
12261230 strip_press: None,
12271231 strip_held: false,
12281232 snapshot: None,
1233 replay_settle: None,
12291234 frames: std::env::var_os("SNOWBOUND_FRAMES").map(|dir| (dir.into(), Instant::now())),
12301235 initial,
12311236 initial_date,
......@@ -1286,6 +1291,9 @@ impl State {
12861291 if let Err(error) = self.open_loaded() {
12871292 eprintln!("{error}");
12881293 }
1294 if let Err(error) = self.take_waiting() {
1295 eprintln!("{error}");
1296 }
12891297 lap("open", start);
12901298 self.follow_reading();
12911299 let size = self.window.inner_size();
......@@ -5706,11 +5714,7 @@ impl State {
57065714 }
57075715 }
57085716 Replay::Snapshot(path) => self.snapshot = Some(path),
5709 Replay::Accessibility(path) => {
5710 if let Err(error) = self.write_accessibility(&path) {
5711 eprintln!("{error}");
5712 }
5713 }
5717 Replay::Settle(path, settled) => self.replay_settle = Some((path, settled)),
57145718 Replay::Tick | Replay::Quit => {}
57155719 Replay::Appearance(appearance) => {
57165720 self.window.set_theme(Some(appearance));
......@@ -5726,6 +5730,16 @@ impl State {
57265730 if let Err(error) = self.frame() {
57275731 eprintln!("{error}");
57285732 }
5733 if self.settled()
5734 && let Some((path, settled)) = self.replay_settle.take()
5735 {
5736 if let Some(path) = path
5737 && let Err(error) = self.write_accessibility(&path)
5738 {
5739 eprintln!("{error}");
5740 }
5741 let _ = settled.send(());
5742 }
57295743 }
57305744 #[cfg(not(target_arch = "wasm32"))]
57315745 UserEvent::Accessibility(event) => self.access_event(event),
......@@ -6041,10 +6055,12 @@ fn write_png(path: &Path, size: [u32; 2], pixels: &[u8]) -> Result<(), Box<dyn E
60416055/// Feeds a development script to the window from another thread, one command per line
60426056/// in logical pixels: `move X Y`, `press [right]`, `release [right]`, `wheel DX DY`,
60436057/// `pressure LEVEL|none`, `pinch FACTOR`, `key NAME`, `type TEXT`, `modifiers [shift]
6044/// [control] [command]`, `wait MILLISECONDS`, `snapshot PNG_PATH`, `accessibility TEXT_PATH`,
6045/// `appearance light|dark`, `resize WIDTH HEIGHT` and `quit`.
6058/// [control] [command]`, `wait MILLISECONDS`, `snapshot PNG_PATH`, `settle` and
6059/// `accessibility TEXT_PATH` (which wait for what is on its way), `appearance light|dark`,
6060/// `resize WIDTH HEIGHT` and `quit`.
60466061#[cfg(not(target_arch = "wasm32"))]
60476062fn replay(script: String, proxy: EventLoopProxy<UserEvent>) -> Result<(), Box<dyn Error>> {
6063 let (settle, settled) = std::sync::mpsc::channel();
60486064 let mut steps = Vec::new();
60496065 for line in script.lines().filter(|line| !line.trim().is_empty()) {
60506066 let (command, rest) = line.split_once(' ').unwrap_or((line, ""));
......@@ -6113,7 +6129,8 @@ fn replay(script: String, proxy: EventLoopProxy<UserEvent>) -> Result<(), Box<dy
61136129 .map_err(|_| "resize takes WIDTH HEIGHT")?,
61146130 )),
61156131 "snapshot" => Ok(Replay::Snapshot(rest.into())),
6116 "accessibility" => Ok(Replay::Accessibility(rest.into())),
6132 "settle" => Ok(Replay::Settle(None, settle.clone())),
6133 "accessibility" => Ok(Replay::Settle(Some(rest.into()), settle.clone())),
61176134 "appearance" => Ok(Replay::Appearance(match rest {
61186135 "light" => winit::window::Theme::Light,
61196136 "dark" => winit::window::Theme::Dark,
......@@ -6129,7 +6146,18 @@ fn replay(script: String, proxy: EventLoopProxy<UserEvent>) -> Result<(), Box<dy
61296146 if let Replay::Input(ui::Event::Button { at, .. }) = &mut replay {
61306147 *at = Instant::now();
61316148 }
6149 let settling = matches!(replay, Replay::Settle(..));
61326150 let _ = proxy.send_event(UserEvent::Replay(replay));
6151 // Frames tick until settled; a minute bounds a page that never lands.
6152 let start = Instant::now();
6153 while settling
6154 && start.elapsed().as_secs() < 60
6155 && settled
6156 .recv_timeout(std::time::Duration::from_micros(16_667))
6157 .is_err()
6158 {
6159 let _ = proxy.send_event(UserEvent::Replay(Replay::Tick));
6160 }
61336161 }
61346162 Err(duration) => {
61356163 // Ticks keep a 60 Hz display's pace, dropping those a slow frame missed.
crates/snowbound/src/undo.rs+48-24
......@@ -23,8 +23,10 @@ pub struct Timeline {
2323 redo: Vec<Step>,
2424 /// Editors of pages left holding history, the longest left first.
2525 parked: Vec<(ExGuid, CanvasEditor)>,
26 /// Actions on their way, while which Undo and Redo wait.
26 /// Actions on their way.
2727 busy: usize,
28 /// Presses of Undo, or Redo when true, that wait for an action or page on its way.
29 waiting: std::collections::VecDeque<bool>,
2830}
2931
3032enum Step {
......@@ -185,9 +187,6 @@ impl Timeline {
185187 depth: [usize; 2],
186188 here: &Here,
187189 ) -> Option<Next> {
188 if self.busy > 0 {
189 return None;
190 }
191190 self.edited(page, depth, false);
192191 let steps = if redo { &mut self.redo } else { &mut self.undo };
193192 let at = steps.iter().rposition(|step| match step {
......@@ -235,10 +234,9 @@ impl Timeline {
235234 /// Whether Undo, or `redo` Redo, has an action to take `here`.
236235 pub fn reaches(&self, redo: bool, here: &Here) -> bool {
237236 let steps = if redo { &self.redo } else { &self.undo };
238 self.busy == 0
239 && steps
240 .iter()
241 .any(|step| matches!(step, Step::Action(action) if action.applies(here)))
237 steps
238 .iter()
239 .any(|step| matches!(step, Step::Action(action) if action.applies(here)))
242240 }
243241
244242 /// Keeps the editor of page `page`, just left, for its history.
......@@ -815,21 +813,47 @@ impl State {
815813 })
816814 }
817815
818 /// Whether Undo, or `redo` Redo, has something to take.
816 /// Whether no action, page or command is on its way, which Undo and Redo wait for.
817 pub(crate) fn settled(&self) -> bool {
818 self.undo.busy == 0 && self.switching.is_none() && self.commands.is_empty()
819 }
820
821 /// Whether Undo, or `redo` Redo, has something to take, or may once what is on its way
822 /// lands.
819823 pub(crate) fn can_step(&self, redo: bool) -> bool {
820824 let editor = &self.view.editor;
821 (if redo {
822 editor.can_redo()
823 } else {
824 editor.can_undo()
825 }) || self
826 .here()
827 .is_some_and(|here| self.undo.reaches(redo, &here))
825 !self.settled()
826 || (if redo {
827 editor.can_redo()
828 } else {
829 editor.can_undo()
830 })
831 || self
832 .here()
833 .is_some_and(|here| self.undo.reaches(redo, &here))
834 }
835
836 /// Undo, or `redo` Redo, once what is on its way lands.
837 pub(crate) fn step(&mut self, redo: bool) -> Result<(), Box<dyn Error>> {
838 self.undo.waiting.push_back(redo);
839 self.take_waiting()
840 }
841
842 /// Takes the Undo and Redo presses waiting, while nothing is on its way.
843 pub(crate) fn take_waiting(&mut self) -> Result<(), Box<dyn Error>> {
844 while self.settled()
845 && let Some(redo) = self.undo.waiting.pop_front()
846 {
847 if self.can_step(redo) {
848 self.take(redo)?;
849 }
850 }
851 Ok(())
828852 }
829853
830854 /// Undo, or `redo` Redo: the open page's last edit or the last action made where the
831855 /// user is, whichever came last.
832 pub(crate) fn step(&mut self, redo: bool) -> Result<(), Box<dyn Error>> {
856 fn take(&mut self, redo: bool) -> Result<(), Box<dyn Error>> {
833857 self.persist()?;
834858 let depth = self.view.editor.history_depth();
835859 // A page kept in no notebook has only its own history.
......@@ -876,7 +900,7 @@ impl State {
876900 .ok_or("That notebook is closed")?;
877901 let Some((structure, undo)) = restructuring(&library, &change) else {
878902 return match taken {
879 Some(redo) => self.step(redo),
903 Some(redo) => self.take(redo),
880904 None => Ok(()),
881905 };
882906 };
......@@ -895,6 +919,7 @@ impl State {
895919 let shown = session.space;
896920 let proxy = self.proxy.clone();
897921 self.undo.busy += 1;
922 self.switching = Some((None, web_time::Instant::now()));
898923 self.load(move || {
899924 let applied = site.change(change, shown);
900925 let show = match &applied {
......@@ -908,7 +933,10 @@ impl State {
908933 let Some(applied) = applied else {
909934 // What it would change is gone: Undo goes on to the step before.
910935 return match taken {
911 Some(redo) if stale => state.step(redo),
936 Some(redo) if stale => {
937 state.undo.waiting.push_front(redo);
938 Ok(())
939 }
912940 _ => Ok(()),
913941 };
914942 };
......@@ -1079,8 +1107,7 @@ mod tests {
10791107 assert!(!timeline.reaches(false, &here()));
10801108 }
10811109
1082 /// Redo takes back the last undone first; a new edit ends what actions it held, and
1083 /// waits while an action is on its way.
1110 /// Redo takes back the last undone first; a new edit ends what actions it held.
10841111 #[test]
10851112 fn redo_mirrors_undo_and_new_edits_end_it() {
10861113 let mut timeline = Timeline::default();
......@@ -1098,9 +1125,6 @@ mod tests {
10981125 timeline.stepped(shown, true);
10991126 timeline.stepped(shown, false);
11001127 assert!(!timeline.reaches(true, &here()));
1101 timeline.record(created(page(3)));
1102 timeline.busy = 1;
1103 assert!(timeline.next(false, shown, [0, 1], &here()).is_none());
11041128 }
11051129
11061130 const AUTHOR: &str = "Rust Author";
crates/snowbound/tests/replay.rs+1-1
......@@ -205,7 +205,7 @@ fn undo_walks_back_through_new_pages_and_their_titles() {
205205 let mut steps = Vec::new();
206206 for title in ["type One", "type Two"] {
207207 steps.extend(chord("modifiers command", "key n"));
208 steps.extend([title, "wait 300"].map(String::from));
208 steps.extend(["settle", title, "wait 300"].map(String::from));
209209 }
210210 steps.extend(chord("modifiers command control", "key Left"));
211211 steps.push("accessibility back".into());