M8: the Annotation tool checks its target first, tool.set asks for the note too, one Developer Mode tool list (#2143 review)

tool.annotation validates a named id or ids before changing the tool: a page or unknown layer is an error, and a named layer is always selected. Choosing the tool with tool.set (the toolbar) asks for the selected layer's note like Shift+T. annotation.new only sets the focus request for a selected, non-page layer, so it cannot wait and take the keyboard later. ToolId::works_in_dev_mode is the single list behind the pointer routing, the command allowlist and the toolbar. The press never picks the page. Tests for the invalid targets, the toolbar path, a drag, the re-focus, and a request for an unselected layer.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Model-Effort: Opus 5.5 triaged and briefed; Sonnet 5.5 verified and wrote at medium

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> {
         ToolId::ALL.into_iter().find(|t| t.id().eq_ignore_ascii_case(s))
     }

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(())

crates/engine/src/cmd/view.rs

@@ -235,6 +235,26 @@ pub(crate) fn tool(s: &mut Session, t: ToolId) -> Result<Value> {
     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<Value> {
+    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<CommandSpec> {
     vec![
@@ -536,6 +556,9 @@ pub fn specs() -> Vec<CommandSpec> {
                 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<CommandSpec> {
         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.

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),
     }

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 [

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);
                     }

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");
 }

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()}));