diff --git a/crates/engine/src/canvas/tools.rs b/crates/engine/src/canvas/tools.rs index 75e3fe972..a80e6d41b 100644 --- a/crates/engine/src/canvas/tools.rs +++ b/crates/engine/src/canvas/tools.rs @@ -132,6 +132,11 @@ impl ToolId { ToolId::Stamp => "stamp", } } + /// Whether Developer Mode keeps this tool: it only inspects, so the tools that edit leave it. + /// The one list behind the engine's pointer routing and command allowlist and the toolbar. + pub fn works_in_dev_mode(self) -> bool { + matches!(self, ToolId::Move | ToolId::Hand | ToolId::Comment | ToolId::Annotation) + } pub fn from_id(s: &str) -> Option { ToolId::ALL.into_iter().find(|t| t.id().eq_ignore_ascii_case(s)) } diff --git a/crates/engine/src/canvas/tools/pointer.rs b/crates/engine/src/canvas/tools/pointer.rs index 356ded1dd..d864be264 100644 --- a/crates/engine/src/canvas/tools/pointer.rs +++ b/crates/engine/src/canvas/tools/pointer.rs @@ -103,7 +103,7 @@ impl Session { /// The tool pointer events go to: in Dev Mode every tool but the hand and comments selects. fn acting_tool(&self) -> ToolId { match self.tool.tool { - t if self.tool.dev_mode && !matches!(t, ToolId::Hand | ToolId::Comment | ToolId::Annotation) => ToolId::Move, + t if self.tool.dev_mode && !t.works_in_dev_mode() => ToolId::Move, t => t, } } @@ -206,7 +206,8 @@ impl Session { fn annotation_down(&mut self, ev: &PointerEvent) -> Result<()> { let ds = self.ds()?; let (doc, page) = (ds.doc.clone(), ds.page); - let Some(id) = self.hit(&doc, page, ev.p(), ev.tol() * 2.0) else { return Ok(()) }; + // The same reach as the Move tool's pick (`ev.tol() * 2.0`); never the page itself. + let Some(id) = self.hit(&doc, page, ev.p(), ev.tol() * 2.0).filter(|i| doc.node(*i).is_some_and(|n| !n.is_page())) else { return Ok(()) }; self.set_selection(vec![id]); self.request_ui("annotation.new", json!({"id": id})); Ok(()) diff --git a/crates/engine/src/cmd/view.rs b/crates/engine/src/cmd/view.rs index 3f13169de..fcfd70d44 100644 --- a/crates/engine/src/cmd/view.rs +++ b/crates/engine/src/cmd/view.rs @@ -235,6 +235,26 @@ pub(crate) fn tool(s: &mut Session, t: ToolId) -> Result { Ok(json!({"tool": t.id()})) } +/// Shift+T and choosing the Annotation tool: with a layer selected (or named by `id`/`ids`) the UI +/// is asked for its note at once; in the tool, a press on a layer selects it and asks (the engine +/// never writes the note). A named target is checked first: one that isn't a layer in the document +/// is an error and the tool stays as it was. +fn annotation(s: &mut Session, p: &Value) -> Result { + let named = p.get("id").is_some_and(|v| !v.is_null()) || p.get("ids").is_some_and(|v| !v.is_null()); + let first = crate::params::targets(s, p).into_iter().find(|i| s.doc().is_ok_and(|d| d.node(*i).is_some_and(|n| !n.is_page()))); + if named && first.is_none() { + return Err(bad("tool.annotation", "`id` must name a layer in this document (not a page)".to_string())); + } + let r = tool(s, ToolId::Annotation)?; + if let Some(id) = first { + if named { + crate::cmd::edit::select_and_page(s, vec![id])?; + } + s.request_ui("annotation.new", json!({"id": id})); + } + Ok(r) +} + /// The View menu's commands and their parameters. pub fn specs() -> Vec { vec![ @@ -536,6 +556,9 @@ pub fn specs() -> Vec { if t.on_boards() { crate::board::require(s, &format!("tool.{}", t.id()))?; } + if t == ToolId::Annotation { + return annotation(s, &json!({})); + } tool(s, t) } ), @@ -640,17 +663,7 @@ pub fn specs() -> Vec { spec!("tool.comment", "Comment", "{}", false, |s, _| tool(s, ToolId::Comment)), // Shift+T: with a layer selected (or an `id`) the UI is asked for its note at once; in the // tool, a press on a layer selects it and asks (the engine never writes the note). - spec!("tool.annotation", "Annotation", "{id?}", false, |s, p| { - let r = tool(s, ToolId::Annotation)?; - let first = crate::params::targets(s, p).into_iter().find(|i| s.doc().is_ok_and(|d| d.node(*i).is_some_and(|n| !n.is_page()))); - if let Some(id) = first { - if p.get("id").is_some() { - crate::cmd::edit::select_and_page(s, vec![id])?; - } - s.request_ui("annotation.new", json!({"id": id})); - } - Ok(r) - }), + spec!("tool.annotation", "Annotation", "{id?|ids?}", false, annotation), spec!("tool.shapeBuilder", "Shape builder", "{}", false, |s, _| tool(s, ToolId::ShapeBuilder)), // `apply: false` is for the UI colour picker: the eyedropper only samples (into // `ToolState::picked`) and the picker writes the colour to the paint it edits. diff --git a/crates/engine/src/dev/devinspect.rs b/crates/engine/src/dev/devinspect.rs index 80b123bbb..5ac74b9ee 100644 --- a/crates/engine/src/dev/devinspect.rs +++ b/crates/engine/src/dev/devinspect.rs @@ -230,7 +230,7 @@ pub fn dev_mode_allows(id: &str) -> bool { // Edits recorded as one grouped undo step, so their spec says no history of its own. const GROUPED: [&str; 1] = ["data.fill"]; match id.strip_prefix("tool.") { - Some(t) => matches!(t, "move" | "hand" | "comment" | "annotation"), + Some(t) => crate::tools::ToolId::from_id(t).is_some_and(crate::tools::ToolId::works_in_dev_mode), None if id == "file.revert" || GROUPED.contains(&id) => false, None => OWN.iter().any(|p| id.starts_with(p)) || crate::cmd::find_command(id).is_none_or(|c| !c.history), } diff --git a/crates/engine/src/devmode_behaviour_tests.rs b/crates/engine/src/devmode_behaviour_tests.rs index 7f08179df..7414a297b 100644 --- a/crates/engine/src/devmode_behaviour_tests.rs +++ b/crates/engine/src/devmode_behaviour_tests.rs @@ -149,6 +149,52 @@ fn the_annotation_tool_picks_a_layer_and_asks_for_its_note() { assert_eq!(s.selection(), vec![r]); } +/// A named target that isn't a layer (a page, an unknown id) is an error, and the tool is left as it +/// was; several targets pick the first layer among them and select it. +#[test] +fn the_annotation_tool_checks_the_layer_it_is_named_for_before_changing_anything() { + let mut s = Session::new(); + let f = frame(&mut s, 0.0, "Screen"); + let r = rect(&mut s, f, 20.0); + let page = s.page().unwrap(); + assert!(s.execute("tool.annotation", &json!({"id": page})).is_err(), "a page is not a layer"); + assert!(s.execute("tool.annotation", &json!({"id": "9999:9999"})).is_err(), "no such layer"); + assert!(s.execute("tool.annotation", &json!({"ids": []})).is_err(), "nothing named"); + assert_eq!(s.tool.tool, ToolId::Move, "the tool did not change"); + assert!(note_requests(&s).is_empty()); + // The page first, then the rectangle: the rectangle is the target, and selected. + ex(&mut s, "tool.annotation", json!({"ids": [page, r]})); + assert_eq!((s.tool.tool, s.selection(), note_requests(&s)), (ToolId::Annotation, vec![r], vec![r])); +} + +/// Choosing the tool with `tool.set` (the toolbar) asks for the selected layer's note like Shift+T. +#[test] +fn choosing_the_annotation_tool_from_the_toolbar_asks_for_the_selected_layers_note() { + let mut s = Session::new(); + let f = frame(&mut s, 0.0, "Screen"); + let r = rect(&mut s, f, 20.0); + ex(&mut s, "select.set", json!({"ids": [r]})); + ex(&mut s, "tool.set", json!({"tool": "annotation"})); + assert_eq!((s.tool.tool, note_requests(&s)), (ToolId::Annotation, vec![r])); +} + +/// A drag with the tool never moves the layer it starts on, in Developer Mode or out of it. +#[test] +fn dragging_with_the_annotation_tool_moves_nothing() { + for dev_mode in [true, false] { + let mut s = Session::new(); + let f = frame(&mut s, 0.0, "Screen"); + rect(&mut s, f, 20.0); + s.tool.dev_mode = dev_mode; + ex(&mut s, "select.set", json!({"ids": []})); + ex(&mut s, "tool.annotation", json!({})); + let (doc_before, undo_before) = (s.doc().unwrap().clone(), s.undo_labels(1000).len()); + drag(&mut s, (40.0, 40.0), (140.0, 90.0)); + assert_eq!(*s.doc().unwrap(), doc_before, "dev_mode {dev_mode}"); + assert_eq!(s.undo_labels(1000).len(), undo_before, "dev_mode {dev_mode}"); + } +} + #[test] fn dev_mode_runs_its_own_edits_and_refuses_design_edits_from_menus_and_shortcuts() { for id in [ diff --git a/crates/ui-egui/src/app/commands.rs b/crates/ui-egui/src/app/commands.rs index e0481f3fc..e36471b42 100644 --- a/crates/ui-egui/src/app/commands.rs +++ b/crates/ui-egui/src/app/commands.rs @@ -269,7 +269,11 @@ impl FigApp { "annotation.new" => { // The Annotation tool: show the Inspect tab (never toggle it away) and put the // caret in the layer's new-note field. - if let Some(id) = r.data.get("id").and_then(figcraft_engine::params::id_of) { + // Only a selected layer's field can show; a request for anything else would wait + // and take the keyboard later. + let shown = + |id| self.session.selection().contains(&id) && self.session.doc().is_ok_and(|d| d.node(id).is_some_and(|n| !n.is_page())); + if let Some(id) = r.data.get("id").and_then(figcraft_engine::params::id_of).filter(|id| shown(*id)) { self.ui.right_tab = RightTab::Inspect; self.ui.dev.focus_new_note = Some(id); } diff --git a/crates/ui-egui/src/dev_mode_tests.rs b/crates/ui-egui/src/dev_mode_tests.rs index 844fe1bfe..b7a9f129d 100644 --- a/crates/ui-egui/src/dev_mode_tests.rs +++ b/crates/ui-egui/src/dev_mode_tests.rs @@ -210,5 +210,33 @@ fn the_annotation_tool_opens_the_inspect_tab_with_the_note_field_focused() { assert_eq!(u.app.ui.dev.focus_new_note, None, "the focus request is spent"); // Asking again while already on the Inspect tab does not toggle it away. u.run("tool.annotation", json!({"id": r})); + u.frames(2, Vec::new); assert_eq!(u.app.ui.right_tab, crate::RightTab::Inspect); + assert!(u.ctx.memory(|m| m.has_focus(field)), "the field has the caret again"); + assert_eq!(u.app.ui.dev.focus_new_note, None, "and the request is spent"); +} + +/// A request for a layer that isn't selected (or is gone) does not wait to take the keyboard later. +#[test] +fn a_note_request_for_a_layer_that_is_not_selected_is_dropped() { + let (mut u, _, r) = setup(); + u.app.session.execute("select.set", &json!({"ids": []})).unwrap(); + u.frames(2, Vec::new); + u.app.session.request_ui("annotation.new", json!({"id": r})); + u.frames(2, Vec::new); + assert_eq!(u.app.ui.dev.focus_new_note, None); +} + +/// Choosing the Annotation tool from the toolbar on the Inspect tab keeps the tab (an editing tool +/// would leave it for Design) and asks for the selected layer's note. +#[test] +fn the_toolbar_keeps_the_inspect_tab_for_the_annotation_tool() { + let (mut u, _, r) = setup(); + u.app.session.execute("select.set", &json!({"ids": [r]})).unwrap(); + crate::toolbar::choose(&mut u.app, figcraft_engine::ToolId::Annotation); + u.frames(2, Vec::new); + assert_eq!(u.app.ui.right_tab, crate::RightTab::Inspect); + assert!(u.ctx.memory(|m| m.has_focus(egui::Id::unique(("annot_new", r)).with("te"))), "the note field has the caret"); + crate::toolbar::choose(&mut u.app, figcraft_engine::ToolId::Rectangle); + assert_eq!(u.app.ui.right_tab, crate::RightTab::Design, "a drawing tool leaves it"); } diff --git a/crates/ui-egui/src/toolbar.rs b/crates/ui-egui/src/toolbar.rs index cbe065c22..d57f2019b 100644 --- a/crates/ui-egui/src/toolbar.rs +++ b/crates/ui-egui/src/toolbar.rs @@ -626,8 +626,8 @@ fn flyout(app: &mut FigApp, ui: &mut Ui, g: &Group, bottom_left: egui::Pos2) { /// Choose `tool` from the toolbar. Dev Mode only inspects: a tool that edits leaves it for the /// design. -fn choose(app: &mut FigApp, tool: ToolId) { - if app.ui.right_tab == crate::RightTab::Inspect && !matches!(tool, ToolId::Move | ToolId::Hand | ToolId::Comment | ToolId::Annotation) { +pub(crate) fn choose(app: &mut FigApp, tool: ToolId) { + if app.ui.right_tab == crate::RightTab::Inspect && !tool.works_in_dev_mode() { app.ui.right_tab = crate::RightTab::Design; } app.run("tool.set", json!({"tool": tool.id()}));