Merge pull request #6398 from Hassan1305/feat-4936-context-menu-followups

Part of #4936: follow-ups to #6385 (only the topmost type's word; dictionary tests one at a time)

crates/engine/src/tests/spelling.rs

@@ -6,6 +6,14 @@ use serde_json::{Value, json};
 use crate::cmd::spelling::MAX_DICTIONARY_FILE_BYTES;
 use crate::*;
 
+/// Held by each test that changes the custom dictionary, which the whole process shares: a save
+/// writes every word in it to the test's file and a reload takes the file's words back, so another
+/// test's word could come back after that test deleted it.
+fn dictionary_lock() -> std::sync::MutexGuard<'static, ()> {
+    static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
+    LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner)
+}
+
 /// A session with a new 400 × 400 document.
 fn session() -> Session {
     let mut s = Session::new();
@@ -115,6 +123,7 @@ fn marked(s: &mut Session) -> Vec<String> {
 
 #[test]
 fn added_words_are_right_in_every_check_until_deleted() {
+    let _one_at_a_time = dictionary_lock();
     let mut s = session();
     text(&mut s, 50.0, "drawing Zqvwkx tools");
     assert_eq!(marked(&mut s), ["Zqvwkx"]);
@@ -137,6 +146,7 @@ fn added_words_are_right_in_every_check_until_deleted() {
 
 #[test]
 fn a_changed_word_replaces_the_old_one() {
+    let _one_at_a_time = dictionary_lock();
     let mut s = session();
     text(&mut s, 50.0, "xkvzqa xkvzqb");
     run(&mut s, "text.spelling.addWord", json!({"word": "xkvzqa"}));
@@ -150,6 +160,7 @@ fn a_changed_word_replaces_the_old_one() {
 
 #[test]
 fn the_custom_dictionary_file_is_read_and_kept_up_to_date() {
+    let _one_at_a_time = dictionary_lock();
     let dir = vectorcraft_testkit::temp_dir("spelling-dictionary");
     let file = dir.join("Custom Dictionary.txt");
     std::fs::write(&file, "kqzvwx\nnot one word\n\n").unwrap();
@@ -170,6 +181,7 @@ fn the_custom_dictionary_file_is_read_and_kept_up_to_date() {
 
 #[test]
 fn ignore_all_skips_a_word_in_the_active_document_only() {
+    let _one_at_a_time = dictionary_lock();
     let mut s = session();
     text(&mut s, 50.0, "Wqzxkv drawing wqzxkv vqzxkw");
     run(&mut s, "text.spelling.ignoreAll", json!({"word": "WQZXKV"}));
@@ -183,6 +195,7 @@ fn ignore_all_skips_a_word_in_the_active_document_only() {
 
 #[test]
 fn custom_dictionary_commands_refuse_what_isnt_one_word() {
+    let _one_at_a_time = dictionary_lock();
     let mut s = session();
     let long = "q".repeat(65);
     for (id, p) in [
@@ -210,6 +223,7 @@ fn in_dictionary(s: &mut Session, word: &str) -> bool {
 
 #[test]
 fn a_dictionary_file_that_cant_be_read_whole_is_never_written_over() {
+    let _one_at_a_time = dictionary_lock();
     let dir = vectorcraft_testkit::temp_dir("spelling-unreadable");
     // A folder where the file should be can't be read as one; a file past what craft-proof reads
     // would lose its other words if written back.
@@ -231,6 +245,7 @@ fn a_dictionary_file_that_cant_be_read_whole_is_never_written_over() {
 
 #[test]
 fn a_change_that_cant_be_saved_is_taken_back() {
+    let _one_at_a_time = dictionary_lock();
     let mut s = session();
     run(&mut s, "text.spelling.addWord", json!({"word": "wkqzvx"}));
     let dir = vectorcraft_testkit::temp_dir("spelling-rollback");
@@ -248,6 +263,7 @@ fn a_change_that_cant_be_saved_is_taken_back() {
 #[cfg(unix)]
 #[test]
 fn a_word_another_window_saved_stays_when_this_save_fails() {
+    let _one_at_a_time = dictionary_lock();
     use std::os::unix::fs::PermissionsExt;
     let dir = vectorcraft_testkit::temp_dir("spelling-readonly");
     let file = dir.join("Custom Dictionary.txt");
@@ -296,6 +312,7 @@ fn misspellings_of_one_object_are_what_the_check_lists() {
 /// what Auto Spell Check found before is checked again.
 #[test]
 fn custom_dictionary_changes_move_its_generation_on() {
+    let _one_at_a_time = dictionary_lock();
     use crate::cmd::spelling::dictionary_generation;
     let mut s = session();
     let mut last = dictionary_generation();

crates/ui-egui/src/canvas/tool_overlays.rs

@@ -166,41 +166,55 @@ fn auto_spell_checks(n: &Node) -> bool {
     !shown(&n.appearance.effects) && !n.appearance.items.iter().any(|i| shown(i.effects()))
 }
 
-/// The word with Auto Spell Check's red line at `p` (document space), on top of the others: in
-/// the topmost visible, unlocked type object whose unknown word's glyphs `p` is on, with what its
-/// language's dictionary suggests instead. For the context menu's suggestions, Ignore All and Add
+/// The word with Auto Spell Check's red line at `p` (document space): in the topmost visible type
+/// object whose glyphs `p` is on, when that object isn't locked and Auto Spell Check marks it and
+/// its word at `p` is one it underlines, with what its language's dictionary suggests instead. For the context menu's suggestions, Ignore All and Add
 /// to Dictionary ([Check and correct spelling](https://helpx.adobe.com/illustrator/desktop/design-with-text/edit-format-text/check-and-correct-spelling.html)).
 pub(crate) fn misspelled_word_at(app: &mut VectorcraftApp, p: Point) -> Option<crate::state::ContextSpelling> {
     let st = app.session.active()?;
-    let mut hit = None;
-    fn walk(nodes: &[std::sync::Arc<Node>], f: &mut impl FnMut(&Node)) {
-        for n in nodes.iter().filter(|n| n.visible && !n.locked) {
-            f(n);
-            if let Some(children) = n.children() {
-                walk(children, f);
+    // The topmost type whose glyphs `p` is on, and whether it or a layer or group holding it is
+    // locked: a word under other type isn't the one right-clicked, whether that type's word there
+    // is right or not.
+    let mut top: Option<(vectorcraft_doc::NodeId, bool)> = None;
+    // Back to front as drawn at `p`: an Intertwine group's members by their tier there, those of
+    // one tier in their stacking order ([`vectorcraft_doc::intertwine`]).
+    fn walk(nodes: &[std::sync::Arc<Node>], p: Point, locked: bool, f: &mut impl FnMut(&Node, bool)) {
+        for n in nodes.iter().filter(|n| n.visible) {
+            let locked = locked || n.locked;
+            f(n, locked);
+            let Some(children) = n.children() else { continue };
+            if matches!(n.kind, NodeKind::Intertwine { .. }) {
+                let mut drawn = children.to_vec();
+                drawn.sort_by_key(|c| vectorcraft_doc::intertwine::tier_at(c, p));
+                walk(&drawn, p, locked, f);
+            } else {
+                walk(children, p, locked, f);
             }
         }
     }
-    // Paint order: a later hit is on top.
-    walk(&st.doc.layers, &mut |n| {
+    // A later hit is on top.
+    walk(&st.doc.layers, p, false, &mut |n, locked| {
         let NodeKind::Text(tx) = &n.kind else { return };
-        if !auto_spell_checks(n) || n.geometric_bounds().is_none_or(|b| !b.contains(p)) {
-            return;
-        }
-        let (words, _) = vectorcraft_engine::cmd::spelling::misspellings(tx, &st.spelling_ignored, MAX_UNDERLINED);
-        if words.is_empty() {
+        if n.geometric_bounds().is_none_or(|b| !b.contains(p)) {
             return;
         }
         let lay = vectorcraft_text::layout(vectorcraft_text::FontDb::global(), tx);
-        for w in words {
-            let on = vectorcraft_text::selection_quads(&lay, w.range.start, w.range.end).iter().any(|q| in_quad(q.map(|c| tx.xf * c), p));
-            if on {
-                hit = Some((n.id, w));
-            }
+        let on = vectorcraft_text::selection_quads(&lay, 0, tx.plain_text().len()).iter().any(|q| in_quad(q.map(|c| tx.xf * c), p));
+        if on {
+            top = Some((n.id, locked));
         }
     });
-    let (id, w) = hit?;
-    let Some(NodeKind::Text(tx)) = st.doc.node(id).map(|n| &n.kind) else { return None };
+    let (id, locked) = top?;
+    let n = st.doc.node(id)?;
+    let NodeKind::Text(tx) = &n.kind else { return None };
+    if locked || !auto_spell_checks(n) {
+        return None;
+    }
+    let (words, _) = vectorcraft_engine::cmd::spelling::misspellings(tx, &st.spelling_ignored, MAX_UNDERLINED);
+    let lay = vectorcraft_text::layout(vectorcraft_text::FontDb::global(), tx);
+    let w = words
+        .into_iter()
+        .find(|w| vectorcraft_text::selection_quads(&lay, w.range.start, w.range.end).iter().any(|q| in_quad(q.map(|c| tx.xf * c), p)))?;
     let word = tx.plain_text().get(w.range.clone())?.to_string();
     let uid = st.uid;
     let suggestions = app

crates/ui-egui/src/tests/contextmenu.rs

@@ -506,3 +506,65 @@ fn no_spelling_items_off_a_misspelled_word_or_with_auto_spell_check_off() {
     let texts = click(&mut app, &ctx, p, PointerButton::Secondary);
     assert!(!has(&texts, "Ignore All"), "off: {texts:?}");
 }
+
+/// A right word in type on top of an underlined word in other type: the right-click is on the
+/// top type, so it lists no spelling items for the word under it.
+#[test]
+fn a_word_under_other_type_is_not_the_one_right_clicked() {
+    let mut app = app();
+    let under = spelled(&mut app, "Qzvxtrwk");
+    let ctx = egui::Context::default();
+    frame(&mut app, &ctx, vec![]);
+    let p = word_at(&app, under, 0, 8);
+    let texts = click(&mut app, &ctx, p, PointerButton::Secondary);
+    assert!(has(&texts, "Ignore All"), "alone, the word is offered: {texts:?}");
+    frame(
+        &mut app,
+        &ctx,
+        vec![Event::Key { key: egui::Key::Escape, physical_key: None, pressed: true, repeat: false, modifiers: Default::default() }],
+    );
+    app.run("text.create", json!({"x": 20, "y": 60, "text": "the cat sat on the mat", "size": 24})).unwrap();
+    app.run("select.none", json!({})).unwrap();
+    let texts = click(&mut app, &ctx, p, PointerButton::Secondary);
+    assert!(!has(&texts, "Ignore All"), "under right type: {texts:?}");
+}
+
+/// In an Intertwine group, the type drawn on top at the right-click is the one whose word is
+/// offered: a member brought to the front there, though earlier in the stacking order.
+#[test]
+fn in_an_intertwine_group_the_type_drawn_on_top_is_the_one_right_clicked() {
+    let mut app = app();
+    let under = spelled(&mut app, "Qzvxtrwk");
+    let over =
+        NodeId(app.run("text.create", json!({"x": 20, "y": 60, "text": "the cat sat on the mat", "size": 24})).unwrap()["id"].as_u64().unwrap());
+    app.run("select.set", json!({"ids": [under.0, over.0]})).unwrap();
+    let g = NodeId(app.run("object.intertwine.make", json!({})).unwrap()["id"].as_u64().unwrap());
+    app.select_tool("selection");
+    app.run("select.none", json!({})).unwrap();
+    let ctx = egui::Context::default();
+    frame(&mut app, &ctx, vec![]);
+    // A point on the word where glyphs of both overlap: Intertwine's areas follow the outlines.
+    let quad = {
+        let st = app.session.active().unwrap();
+        let Some(NodeKind::Text(t)) = st.doc.node(under).map(|n| &n.kind) else { panic!("type") };
+        let lay = vectorcraft_text::layout(vectorcraft_text::FontDb::global(), t);
+        vectorcraft_text::selection_quads(&lay, 0, 8)[0].map(|c| t.xf * c)
+    };
+    let (x0, x1) = (quad.iter().map(|c| c.x).fold(f64::MAX, f64::min), quad.iter().map(|c| c.x).fold(f64::MIN, f64::max));
+    let (y0, y1) = (quad.iter().map(|c| c.y).fold(f64::MAX, f64::min), quad.iter().map(|c| c.y).fold(f64::MIN, f64::max));
+    let overlap = (0..40)
+        .flat_map(|i| (0..20).map(move |j| Point::new(x0 + (x1 - x0) * (i as f64 + 0.5) / 40.0, y0 + (y1 - y0) * (j as f64 + 0.5) / 20.0)))
+        .find(|q| app.run("object.intertwine.arrange", json!({"id": g.0, "at": [q.x, q.y], "order": "back", "member": under.0})).is_ok())
+        .expect("glyphs of both overlap somewhere on the word");
+    let p = screen(&app, overlap.x, overlap.y);
+    let texts = click(&mut app, &ctx, p, PointerButton::Secondary);
+    assert!(!has(&texts, "Ignore All"), "under the right type: {texts:?}");
+    frame(
+        &mut app,
+        &ctx,
+        vec![Event::Key { key: egui::Key::Escape, physical_key: None, pressed: true, repeat: false, modifiers: Default::default() }],
+    );
+    app.run("object.intertwine.arrange", json!({"id": g.0, "at": [overlap.x, overlap.y], "order": "front", "member": under.0})).unwrap();
+    let texts = click(&mut app, &ctx, p, PointerButton::Secondary);
+    assert!(has(&texts, "Ignore All"), "brought to the front there: {texts:?}");
+}