From 147a2af4881d7bbc1d857cf57d191ba94c0522ca Mon Sep 17 00:00:00 2001 From: clover caruso Date: Mon, 12 Oct 2026 16:32:52 -0700 Subject: [PATCH] fix(desktop): route shortcut input before the next keystroke Refs #123, #129 Assisted-by: gpt-6.1-sol --- crates/snowbound/src/commands.rs | 55 ++++- crates/snowbound/src/main.rs | 77 ++++-- crates/snowbound/src/manage.rs | 49 +++- crates/snowbound/src/search.rs | 2 +- crates/snowbound/tests/replay.rs | 395 +++++++++++++++++++++++++++++++ 5 files changed, 534 insertions(+), 44 deletions(-) diff --git a/crates/snowbound/src/commands.rs b/crates/snowbound/src/commands.rs index f75f7bdc838c0a0388b2f3616289e6c8e8dd14f6..03167dd928ae6836ee718a6af4ac27bd823b263e 100644 --- a/crates/snowbound/src/commands.rs +++ b/crates/snowbound/src/commands.rs @@ -1079,6 +1079,43 @@ impl State { self.window.request_redraw(); } + pub(crate) fn choose_input(&mut self, choice: Choice) { + #[cfg(target_os = "macos")] + if !platform::commands_allowed(&choice) { + return; + } + let id = match choice { + Choice::Command(id @ (Id::Find | Id::NewPage | Id::NewSubpage)) => id, + choice => { + self.choose(choice); + return; + } + }; + let result = (|| -> Result<(), Box> { + let size = self.window.inner_size(); + let size = [size.width, size.height] + .map(|side| side as f32 / self.window.scale_factor() as f32); + let scale = self.scale(); + self.layout(size, scale)?; + while !self.commands.is_empty() { + let mut commands = std::mem::take(&mut self.commands).into_iter(); + while let Some(command) = commands.next() { + if let Err(error) = self.apply(command) { + self.commands = + commands.chain(std::mem::take(&mut self.commands)).collect(); + return Err(error); + } + } + } + self.run(Choice::Command(id))?; + self.layout(size, scale) + })(); + if let Err(error) = result { + platform::alert(command(id).title, &crate::plain(&*error, "page")); + } + self.window.request_redraw(); + } + /// Every choice's status, in `choices` order, for the menu bar. Link gives up its chord, /// ⌘K, to the open palette's actions. pub(crate) fn statuses(&self) -> Vec { @@ -1508,14 +1545,16 @@ impl State { }, ) } - Id::NewPage => Work::NewPage { - under: None, - title: String::new(), - }, - Id::NewSubpage => Work::NewPage { - under: self.session.as_ref().map(|session| session.space), - title: String::new(), - }, + Id::NewPage | Id::NewSubpage => { + let under = self + .session + .as_ref() + .filter(|_| id == Id::NewSubpage) + .map(|session| session.space); + self.new_page(under, "")?; + self.edited(Vec::new()); + return Ok(()); + } Id::PageVersions => { let session = self.session.as_ref().ok_or("No notebook is open")?; Work::History { diff --git a/crates/snowbound/src/main.rs b/crates/snowbound/src/main.rs index 9bda34fa4fadf3edd166d7ba07f6efaf58bb50a2..8006b6aea7492c29f094fcc79a60900c09fe3cae 100644 --- a/crates/snowbound/src/main.rs +++ b/crates/snowbound/src/main.rs @@ -390,6 +390,7 @@ impl std::task::Wake for Redraw { #[derive(Debug)] enum Replay { Input(ui::Event), + Inputs(Vec), Access(accesskit::Action, String), /// A trackpad pinch scaling the page by the factor. Pinch(f32), @@ -695,8 +696,7 @@ enum Laid { Library(Arc, Option), } -/// The newest page read, laid out and waiting for the pictures it shows first, so it -/// never appears without them. +/// A page ready for adoption, with the scene and editor laid out together. struct Opening { loaded: Shown, scene: (PageScene, [f32; 2]), @@ -934,8 +934,6 @@ struct State { drag: Option, /// Pages whose template strip was dismissed this run. dismissed: HashSet, - /// A page just created, whose title takes the caret once it opens. - title_focus: Option, /// The page colour a menu previews over the open page's own this frame. page_color_preview: Option>, media: recording::Media, @@ -1347,7 +1345,6 @@ impl State { sidebar: stored.sidebar, folded: HashSet::new(), dismissed: HashSet::new(), - title_focus: None, menu: None, sync_notebook: None, actions: None, @@ -3847,6 +3844,10 @@ impl State { return Ok(()); } lap("switch pictures", opening.since); + self.show_page(opening) + } + + fn show_page(&mut self, mut opening: Opening) -> Result<(), Box> { let requested = opening.requested; if let Shown::Section(session) | Shown::Created(session) = &mut opening.loaded && let Some(library) = self @@ -3931,6 +3932,7 @@ impl State { } Shown::Page(space) | Shown::Version(space, _) => { let session = self.session.as_mut().ok_or("No section is open")?; + let pages = session.section.pages()?; session.space = space; session.version = match opening.loaded { Shown::Version(_, version) => Some(version), @@ -3938,7 +3940,7 @@ impl State { }; session.change = None; // Pages created, moved or deleted since the list was read. - session.pages = session.section.pages()?; + session.pages = pages; leaving = leaving.filter(|left| session.pages.iter().any(|(s, ..)| s == left)); } } @@ -3978,21 +3980,6 @@ impl State { self.opened()?; self.prefetch_around(); self.refind(true)?; - if self.title_focus.take().is_some_and(|space| { - self.session - .as_ref() - .is_some_and(|session| session.space == space) - }) && let Some(title) = self - .view - .editor - .outlines() - .iter() - .find(|outline| outline.title) - { - self.view.editor.focus_outline(title.id)?; - let response = self.view.focus_text()?; - self.respond(response); - } if created && let Some(session) = &self.session { let target = rename::Target::Entry { library: Arc::clone(&session.library), @@ -4913,7 +4900,7 @@ impl State { commands::find(&ui::edit_key(key), ui::edit_modifiers(self.ui.modifiers())) && !(id == commands::Id::Link && self.ui.popup_open(palette::id())) { - self.choose(commands::Choice::Command(id)); + self.choose_input(commands::Choice::Command(id)); } else { self.ui.event(event); } @@ -6385,7 +6372,7 @@ impl State { eprintln!("{error}"); } } - UserEvent::Choose(choice) => self.choose(choice), + UserEvent::Choose(choice) => self.choose_input(choice), UserEvent::Appearance => { self.follow_color_scheme(); self.window.request_redraw(); @@ -6402,6 +6389,14 @@ impl State { }; match replay { Replay::Input(event) => self.input(event), + Replay::Inputs(events) => { + for mut event in events { + if let ui::Event::Button { at, .. } = &mut event { + *at = Instant::now(); + } + self.input(event); + } + } Replay::Access(action, label) => { let tree = self.ui.accessibility_tree("", self.window.scale_factor()); let mut nodes = tree @@ -6813,12 +6808,26 @@ 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(); + let mut batch = None; for line in script.lines().filter(|line| !line.trim().is_empty()) { + if line == "batch" { + if batch.replace(Vec::new()).is_some() { + return Err("Replay input batches cannot nest".into()); + } + continue; + } + if line == "end" { + steps.push(Ok(Replay::Inputs( + batch.take().ok_or("No input batch is open")?, + ))); + continue; + } let (command, rest) = line.split_once(' ').unwrap_or((line, "")); let numbers = || -> Result, std::num::ParseFloatError> { rest.split_whitespace().map(str::parse).collect() @@ -6832,7 +6841,7 @@ fn replay(script: String, proxy: EventLoopProxy) -> Result<(), Box Ok(Replay::Input(ui::Event::PointerMoved( numbers()?.try_into().map_err(|_| "move takes X Y")?, ))), @@ -6865,11 +6874,16 @@ fn replay(script: String, proxy: EventLoopProxy) -> Result<(), Box Ok(Replay::Input(ui::Event::Ime(Ime::Commit(rest.into())))), + "preedit" => Ok(Replay::Input(ui::Event::Ime(Ime::Preedit( + rest.into(), + Some((rest.len(), rest.len())), + )))), "modifiers" => Ok(Replay::Input(ui::Event::Modifiers( rest.split_whitespace() .map(|name| match name { "shift" => Ok(ModifiersState::SHIFT), "control" => Ok(ModifiersState::CONTROL), + "option" => Ok(ModifiersState::ALT), // The shortcut modifier, as ui::edit_modifiers reads it. "command" if cfg!(target_os = "macos") => Ok(ModifiersState::SUPER), "command" => Ok(ModifiersState::CONTROL), @@ -6903,7 +6917,18 @@ fn replay(script: String, proxy: EventLoopProxy) -> Result<(), Box return Err(format!("Unknown appearance {rest}").into()), })), _ => return Err(format!("Unknown replay command: {line}").into()), - }); + }; + if let Some(batch) = &mut batch { + match step { + Ok(Replay::Input(event)) => batch.push(event), + _ => return Err("A replay input batch accepts only input events".into()), + } + } else { + steps.push(step); + } + } + if batch.is_some() { + return Err("Replay input batch is missing end".into()); } std::thread::spawn(move || { for step in steps { diff --git a/crates/snowbound/src/manage.rs b/crates/snowbound/src/manage.rs index 697daf1b3db02327314d1d138948e4d8b19152a7..a97f74044a0693d54ae6bf302fe14b2205c06776 100644 --- a/crates/snowbound/src/manage.rs +++ b/crates/snowbound/src/manage.rs @@ -219,21 +219,52 @@ impl State { )?]))); } let session = self.session.as_mut().ok_or("No section is open")?; - session.section.apply( - &self.author, - Edit { - at: crate::filetime(), - ops, - }, - )?; + let edit = Edit { + at: crate::filetime(), + ops, + }; + #[cfg(not(target_arch = "wasm32"))] + session.section.replica().apply(&self.author, edit)?; + #[cfg(target_arch = "wasm32")] + session.section.apply(&self.author, edit)?; let shown = session.space; self.made(Change::Delete { pages: vec![space], show: Some(shown), created: true, }); - self.title_focus = Some(space); - self.commands.push(Command::OpenPage(space)); + let session = self.session.as_mut().ok_or("No section is open")?; + session.pages = session.section.pages()?; + let page = session.section.replica().page(space)?; + let (scene, editor) = canvas::gpu::page::PageScene::from_page(page, &mut self.view.engine)?; + let now = crate::Instant::now(); + let shown = self.show_page(crate::Opening { + loaded: crate::Shown::Page(space), + scene: (scene, [0.0; 2]), + editor, + since: now, + requested: now, + }); + if self + .session + .as_ref() + .is_some_and(|session| session.space == space) + { + self.stop_loading(); + } + shown?; + self.ui.set_focus(Some(crate::page())); + if let Some(title) = self + .view + .editor + .outlines() + .iter() + .find(|outline| outline.title) + { + self.view.editor.focus_outline(title.id)?; + let response = self.view.focus_text()?; + self.respond(response); + } Ok(()) } diff --git a/crates/snowbound/src/search.rs b/crates/snowbound/src/search.rs index 0c46cf366b5fc24657454dcec286ee36420cf43e..6940022e5d609d64f2cd2f3295653fc0a07716e5 100644 --- a/crates/snowbound/src/search.rs +++ b/crates/snowbound/src/search.rs @@ -670,7 +670,7 @@ impl State { self.search.open = false; self.ui.close_popup(results()); self.search.finding = true; - self.search.claim = true; + self.ui.set_focus(Some(find_field())); self.search.current = None; self.refind(true) } diff --git a/crates/snowbound/tests/replay.rs b/crates/snowbound/tests/replay.rs index e8ae0b042a1ba2a2661a52c48fd15e0925851917..07494a2b48a924cdaeb59310cdf3a3a357099934 100644 --- a/crates/snowbound/tests/replay.rs +++ b/crates/snowbound/tests/replay.rs @@ -589,6 +589,401 @@ fn renaming_a_section_from_its_tab_types_into_the_tab_at_once() { assert!(renamed.contains(r#"Tab "Renamed""#), "{renamed}"); } +fn stored_pages(path: &Path) -> Vec<(onestore::ExGuid, onestore::page::Page, u32)> { + let bytes = std::fs::read(path).unwrap(); + let store = onestore::Store::parse(&bytes).unwrap(); + let index = onestore::RevisionIndex::parse(&store).unwrap(); + let document = onestore::document::Document::parse(&index).unwrap(); + document + .pages() + .unwrap() + .into_iter() + .map(|(space, page)| { + ( + space, + onestore::page::Page::from_space(&document, space).unwrap(), + onestore::page::Page::heading(document.active(space).unwrap(), page).1, + ) + }) + .collect() +} + +fn body_text(page: &onestore::page::Page) -> Vec { + page.objects + .iter() + .filter_map(|object| match object { + onestore::page::PageObject::Outline(outline) if !outline.title => Some(outline), + _ => None, + }) + .flat_map(|outline| &outline.paragraphs) + .filter_map(|paragraph| paragraph.text()) + .map(|text| text.text.text().to_owned()) + .collect() +} + +fn assert_replica_matches( + scratch: &Scratch, + pages: &[(onestore::ExGuid, onestore::page::Page, u32)], +) { + let notebook = + notebook::session::Notebook::open(scratch.0.join("notebook"), scratch.0.join("cache")) + .unwrap(); + let replica = notebook::Replica::open(notebook.replica_path("Cross.one").unwrap()).unwrap(); + assert!(replica.pending().unwrap().is_empty()); + assert_eq!( + replica.pages().unwrap(), + pages + .iter() + .map(|(space, page, level)| (*space, page.title.clone(), *level)) + .collect::>() + ); + for (space, page, _) in pages { + assert_eq!(&replica.page(*space).unwrap(), page); + } +} + +#[test] +fn find_takes_the_next_input_without_a_display_frame() { + let scratch = Scratch::new("find-burst"); + let notebook = + Path::new(env!("CARGO_MANIFEST_DIR")).join("../../corpus/cross-container/candidate"); + let original = stored_pages(¬ebook.join("Cross.one")); + let [found] = replay( + &scratch, + Some(¬ebook), + &[ + "batch", + "move 900 600", + "press", + "release", + "preedit 雪", + "type BEFORE_FIND_QA", + "modifiers command", + "key f", + "modifiers", + "type FIND_QUERY_QAX", + "key Backspace", + "end", + "accessibility found", + ], + ) + .try_into() + .unwrap(); + assert!( + found.contains(r#"SearchInput "Find on Page" = "FIND_QUERY_QA" [focused]"#), + "{found}" + ); + let pages = stored_pages(&scratch.0.join("notebook/Cross.one")); + assert_eq!(pages.len(), original.len()); + assert_eq!(pages[0].0, original[0].0); + assert_eq!(pages[0].1.title, original[0].1.title); + assert!( + body_text(&pages[0].1) + .iter() + .any(|text| text == "BEFORE_FIND_QA") + ); + assert!( + body_text(&pages[0].1) + .iter() + .all(|text| !text.contains("FIND_QUERY_QA")) + ); + assert_eq!(&pages[1..], &original[1..]); + assert_replica_matches(&scratch, &pages); +} + +#[test] +fn new_pages_take_burst_titles_and_bodies_before_a_display_frame() { + let scratch = Scratch::new("new-page-burst"); + let notebook = + Path::new(env!("CARGO_MANIFEST_DIR")).join("../../corpus/cross-container/candidate"); + let original = stored_pages(¬ebook.join("Cross.one")); + let [burst, back, forward, undone, redone] = replay( + &scratch, + Some(¬ebook), + &[ + "batch", + "move 900 600", + "press", + "release", + "type BEFORE_NEW_QA", + "modifiers command", + "key n", + "modifiers", + "type FIRST_TITLE_QAX", + "key Backspace", + "key Tab", + "type FIRST_BODY_QA", + "modifiers command", + "key n", + "modifiers", + "type SECOND_TITLE_QA", + "key Tab", + "type SECOND_BODY_QA", + "end", + "accessibility burst", + "modifiers command control", + "key Left", + "modifiers", + "accessibility back", + "modifiers command control", + "key Right", + "modifiers", + "accessibility forward", + "modifiers command", + "key z", + "modifiers", + "accessibility undone", + "modifiers command shift", + "key z", + "modifiers", + "accessibility redone", + ], + ) + .try_into() + .unwrap(); + for tree in [&burst, &forward, &redone] { + assert!( + page_tabs(tree).contains(&"*SECOND_TITLE_QA".to_owned()), + "{tree}" + ); + assert!(tree.contains("SECOND_BODY_QA"), "{tree}"); + assert!( + !tree.contains("FIRST_BODY_QA") && !tree.contains("BEFORE_NEW_QA"), + "{tree}" + ); + } + assert!( + page_tabs(&back).contains(&"*FIRST_TITLE_QA".to_owned()), + "{back}" + ); + assert!(back.contains("FIRST_BODY_QA"), "{back}"); + assert!(!undone.contains("SECOND_BODY_QA"), "{undone}"); + let pages = stored_pages(&scratch.0.join("notebook/Cross.one")); + assert_eq!(pages.len(), original.len() + 2); + assert_eq!(pages[0].0, original[0].0); + assert_eq!(pages[0].1.title, original[0].1.title); + assert!( + body_text(&pages[0].1) + .iter() + .any(|text| text == "BEFORE_NEW_QA") + ); + assert!( + body_text(&pages[0].1) + .iter() + .all(|text| !text.contains("TITLE_QA") && !text.contains("BODY_QA")) + ); + assert_eq!(&pages[1..original.len()], &original[1..]); + for (page, title, body) in [ + (&pages[original.len()].1, "FIRST_TITLE_QA", "FIRST_BODY_QA"), + ( + &pages[original.len() + 1].1, + "SECOND_TITLE_QA", + "SECOND_BODY_QA", + ), + ] { + assert_eq!(page.title, title); + assert_eq!(body_text(page), [body]); + } + assert_ne!(pages[original.len()].0, pages[original.len() + 1].0); + assert_replica_matches(&scratch, &pages); +} + +#[test] +fn back_followed_by_new_page_cannot_replace_the_burst_target() { + let scratch = Scratch::new("back-new-page-burst"); + let notebook = + Path::new(env!("CARGO_MANIFEST_DIR")).join("../../corpus/cross-container/candidate"); + let original = stored_pages(¬ebook.join("Cross.one")); + let [before, created, later] = replay( + &scratch, + Some(¬ebook), + &[ + "batch", + "modifiers command", + "key n", + "modifiers", + "type BACK_FIRST_QA", + "key Tab", + "type BACK_FIRST_BODY_QA", + "modifiers command", + "key n", + "modifiers", + "type BACK_SECOND_QA", + "key Tab", + "type BACK_SECOND_BODY_QA", + "end", + "accessibility before", + "batch", + "modifiers command control", + "key Left", + "modifiers command", + "key n", + "modifiers", + "type BACK_NEW_QA", + "key Tab", + "type BACK_NEW_BODY_QA", + "end", + "accessibility created", + "type _AFTER_SETTLE_QA", + "accessibility later", + ], + ) + .try_into() + .unwrap(); + assert!( + page_tabs(&before).contains(&"*BACK_SECOND_QA".to_owned()), + "{before}" + ); + for tree in [&created, &later] { + assert!( + page_tabs(tree).contains(&"*BACK_NEW_QA".to_owned()), + "{tree}" + ); + assert!(tree.contains("BACK_NEW_BODY_QA"), "{tree}"); + assert!( + !tree.contains("BACK_FIRST_BODY_QA") && !tree.contains("BACK_SECOND_BODY_QA"), + "{tree}" + ); + } + assert!( + later.contains("BACK_NEW_BODY_QA_AFTER_SETTLE_QA"), + "{later}" + ); + let pages = stored_pages(&scratch.0.join("notebook/Cross.one")); + assert_eq!(pages.len(), original.len() + 3); + assert_eq!(&pages[..original.len()], &original); + for (page, title, body) in [ + ( + &pages[original.len()].1, + "BACK_FIRST_QA", + "BACK_FIRST_BODY_QA", + ), + ( + &pages[original.len() + 1].1, + "BACK_SECOND_QA", + "BACK_SECOND_BODY_QA", + ), + ( + &pages[original.len() + 2].1, + "BACK_NEW_QA", + "BACK_NEW_BODY_QA_AFTER_SETTLE_QA", + ), + ] { + assert_eq!(page.title, title); + assert_eq!(body_text(page), [body]); + } + let identities = pages + .iter() + .map(|(space, ..)| *space) + .collect::>(); + assert_eq!(identities.len(), pages.len()); + assert_replica_matches(&scratch, &pages); +} + +#[test] +fn a_new_subpage_takes_the_next_input_and_keeps_its_parent() { + let scratch = Scratch::new("new-subpage-burst"); + let notebook = + Path::new(env!("CARGO_MANIFEST_DIR")).join("../../corpus/cross-container/candidate"); + let original = stored_pages(¬ebook.join("Cross.one")); + let [tree] = replay( + &scratch, + Some(¬ebook), + &[ + "batch", + "modifiers command shift option", + "key n", + "modifiers", + "type SUBPAGE_TITLE_QA", + "key Tab", + "type SUBPAGE_BODY_QA", + "end", + "accessibility subpage", + ], + ) + .try_into() + .unwrap(); + assert!( + page_tabs(&tree).contains(&"*SUBPAGE_TITLE_QA".to_owned()), + "{tree}" + ); + assert!(tree.contains("SUBPAGE_BODY_QA"), "{tree}"); + let pages = stored_pages(&scratch.0.join("notebook/Cross.one")); + assert_eq!(pages.len(), original.len() + 1); + assert_eq!(pages[0], original[0]); + assert_eq!(&pages[2..], &original[1..]); + assert_eq!(pages[1].1.title, "SUBPAGE_TITLE_QA"); + assert_eq!(body_text(&pages[1].1), ["SUBPAGE_BODY_QA"]); + assert_eq!(pages[1].2, (original[0].2 + 1).min(3)); + assert_replica_matches(&scratch, &pages); +} + +#[test] +fn an_open_dialog_keeps_burst_shortcuts_out_of_the_page() { + let scratch = Scratch::new("modal-shortcut-burst"); + let notebook = + Path::new(env!("CARGO_MANIFEST_DIR")).join("../../corpus/cross-container/candidate"); + let original = stored_pages(¬ebook.join("Cross.one")); + let [tree] = replay( + &scratch, + Some(¬ebook), + &[ + "batch", + "modifiers command", + "key ,", + "key f", + "key n", + "modifiers", + "type MODAL_INPUT_QA", + "end", + "accessibility modal", + "key Escape", + ], + ) + .try_into() + .unwrap(); + assert!(tree.contains("Dialog"), "{tree}"); + assert!(!tree.contains("Find on Page"), "{tree}"); + let pages = stored_pages(&scratch.0.join("notebook/Cross.one")); + assert_eq!(pages, original); + assert_replica_matches(&scratch, &pages); +} + +#[test] +fn disabled_shortcuts_leave_an_empty_notebook_empty() { + let scratch = Scratch::new("disabled-shortcut-burst"); + let [tree] = replay( + &scratch, + None, + &[ + "batch", + "modifiers command", + "key f", + "modifiers", + "type QUERY_QA", + "modifiers command", + "key n", + "modifiers", + "type TITLE_QA", + "end", + "accessibility empty", + ], + ) + .try_into() + .unwrap(); + assert!(tree.contains("No sections in this notebook"), "{tree}"); + assert!( + !tree.contains("Find on Page") && !tree.contains("TITLE_QA"), + "{tree}" + ); + assert!( + std::fs::read_dir(scratch.0.join("notebook")) + .unwrap() + .next() + .is_none() + ); +} + /// Finding on the page leaves the query room beside the match count: a press just past /// where the count ends lands in the field. #[test] -- 2.54.0