Merge pull request #1091 from ispyisail/fix/qet-mcp-edit-folio-hint

Fix qet_edit giving no clue when a folio number is off by one

misc/qet-mcp/README.md

@@ -172,7 +172,10 @@ the answer a screenshot gave wrongly.
 ```
 
 An op that creates something takes an `"id"`; later ops name it as `"$id"`.
-Terminals are addressed by index — top to bottom, then left to right, **not**
+A `"folio"` given as a number counts **from 0**, while `qet_elements` and
+`qet_project_info` number folios from 1 as the application does: the folio
+they call 1 is `"folio": 0` here. An op that fails because of this says which
+index to use. Terminals are addressed by index — top to bottom, then left to right, **not**
 the order the `.elmt` lists them. `qet_element_info` and `qet_element_search`
 both report that index order. The answer carries a per-operation result
 *and* a `qet_diff`, because "addConductor → true" says the call was

misc/qet-mcp/qet_mcp.py

@@ -1990,6 +1990,35 @@ def tool_project_new(binary: str, output: str, title: str = "Untitled",
     return result
 
 
+def _wrong_folio(project: str, op: dict) -> str:
+    """Explain a failed op that named an element on the wrong folio.
+
+    qet_elements and qet_project_info number folios from 1, as the UI does;
+    qet_edit passes "folio" straight to the scripting API, which counts from
+    0. Passing the number qet_elements showed therefore addresses the next
+    folio, and the op fails with nothing but "false". When the element the
+    op names is in the project on some other folio, say which index to use.
+    """
+    folio = op.get("folio")
+    spec = OPS.get(op.get("op"), (None, []))[1]
+    uuids = [op[key] for key, kind in spec
+             if kind == "elmt" and isinstance(op.get(key), str)
+             and not op[key].startswith("$")]
+    if not isinstance(folio, int) or not uuids:
+        return ""
+    try:
+        where = {el.get("uuid"): i for i, el in _elements(_root(project))}
+    except (OSError, ET.ParseError):
+        return ""
+    for uuid in uuids:
+        number = where.get(uuid)
+        if number is not None and number - 1 != folio:
+            return (f"Element {uuid} is on folio {number} as qet_elements numbers "
+                    f"it, which is \"folio\": {number - 1} here: qet_edit counts "
+                    "folios from 0.")
+    return ""
+
+
 def tool_edit(binary: str, project: str, operations: list, output: str,
               elements_dir: str | None = None, timeout: int = 180) -> dict:
     """Apply edits through the scripting API and report what actually changed.
@@ -2060,10 +2089,14 @@ def tool_edit(binary: str, project: str, operations: list, output: str,
     for record in result.get("operations", []):
         if not record["succeeded"]:
             result["ok"] = False
-            result.setdefault("hint",
-                              f"operation {record['index']} ({record['op']}) returned "
-                              f"{record['result']!r}; later operations were skipped. "
-                              "qet.log lines in stderr/stdout say why.")
+            hint = (f"operation {record['index']} ({record['op']}) returned "
+                    f"{record['result']!r}; later operations were skipped. "
+                    "qet.log lines in stderr/stdout say why.")
+            if 0 <= record["index"] < len(operations):
+                wrong = _wrong_folio(str(proj), operations[record["index"]])
+                if wrong:
+                    hint += " " + wrong
+            result.setdefault("hint", hint)
             break
 
     if result.get("saved") is False:
@@ -2236,7 +2269,9 @@ TOOLS = [
                         "Give an op an \"id\" to name what it produced, then refer to "
                         "it later as \"$id\" -- that is how an element placed by "
                         "add_element gets wired by add_conductor, and how a folio made "
-                        "by add_folio is addressed. Terminals are numbered by their "
+                        "by add_folio is addressed. A \"folio\" given as a number is "
+                        "an index counted from 0: the folio qet_elements and "
+                        "qet_project_info call 1 is \"folio\": 0 here. Terminals are numbered by their "
                         "index in the element definition; qet_element_info lists them. "
                         "set_conductor addresses a conductor as the one on a given "
                         "terminal and applies the change to its whole electrical "

misc/qet-mcp/test_qet_mcp.py

@@ -670,6 +670,43 @@ class ElementSearch(unittest.TestCase):
         self.assertEqual(m.tool_element_search(str(self.root), "fine")["total_matches"], 1)
 
 
+class WrongFolioHint(unittest.TestCase):
+    """qet_elements numbers folios from 1, qet_edit from 0; a failed op that
+    used the wrong one should say which index to use."""
+
+    def setUp(self):
+        self.tmp = tempfile.TemporaryDirectory()
+        self.qet = Path(self.tmp.name) / "p.qet"
+        self.qet.write_text(
+            '<project><diagram><elements/></diagram>'
+            '<diagram><elements><element uuid="{b}" type="x" x="0" y="0"/></elements></diagram>'
+            '</project>')
+
+    def tearDown(self):
+        self.tmp.cleanup()
+
+    def hint(self, op):
+        return m._wrong_folio(str(self.qet), op)
+
+    def test_the_number_qet_elements_showed_gets_the_index_to_use(self):
+        h = self.hint({"op": "move_element", "folio": 2, "element": "{b}", "dx": 5, "dy": 0})
+        self.assertIn("folio 2 as qet_elements numbers it", h)
+        self.assertIn('"folio": 1 here', h)
+
+    def test_no_hint_when_the_folio_was_right_or_cannot_be_checked(self):
+        for op in ({"op": "move_element", "folio": 1, "element": "{b}"},     # right index
+                   {"op": "move_element", "folio": 0, "element": "{nope}"},  # not in the file
+                   {"op": "move_element", "folio": 0, "element": "$k1"},     # placed by the script
+                   {"op": "move_element", "folio": "$f", "element": "{b}"},  # folio by reference
+                   {"op": "add_folio"}):
+            with self.subTest(op=op):
+                self.assertEqual(self.hint(op), "")
+
+    def test_the_description_says_how_folios_are_counted(self):
+        tool = next(t for t in m.TOOLS if t["name"] == "qet_edit")
+        self.assertIn("counted from 0", tool["inputSchema"]["properties"]["operations"]["description"])
+
+
 class Diff(unittest.TestCase):
     def setUp(self):
         self.tmp = tempfile.TemporaryDirectory()
@@ -2734,6 +2771,24 @@ class CorpusIntegration(unittest.TestCase):
         self.assertGreater(total, 3000)
         self.assertEqual(collisions, 0)
 
+    def test_a_folio_number_from_qet_elements_fails_with_the_index_to_use(self):
+        sb = Sandbox()
+        try:
+            src = shutil.copy(Path(EXAMPLES) / "grafcet.qet", sb.p("g.qet"))
+            first = m.tool_elements(src, folio=1)["elements"][0]
+            self.assertEqual(first["folio"], 1)
+            move = {"op": "move_element", "element": first["uuid"], "dx": 10, "dy": 0}
+            r = m.tool_edit(BINARY, src, [dict(move, folio=first["folio"])], sb.p("wrong.qet"),
+                            elements_dir=ELEMENTS or None)
+            self.assertFalse(r["ok"])
+            self.assertIn('"folio": 0 here', r["hint"])
+            r = m.tool_edit(BINARY, src, [dict(move, folio=0)], sb.p("right.qet"),
+                            elements_dir=ELEMENTS or None)
+            self.assertTrue(r["ok"], r.get("hint"))
+            self.assertEqual(r["diff"]["elements"]["distinct_move_deltas"], [[10.0, 0.0]])
+        finally:
+            sb.close()
+
     def test_untouched_resave_diffs_clean_on_every_small_example(self):
         sb = Sandbox()
         try: