From 9843d75c910f9dd3de35d02d5c9989741d5cf494 Mon Sep 17 00:00:00 2001 From: Matthew Runo <74583+inktomi@users.noreply.github.com> Date: Mon, 18 May 2026 11:05:14 -0700 Subject: [PATCH] fix(add_schematic_component): support hierarchical sub-sheets (#169) DynamicSymbolLoader.create_component_instance only handled root schematics: it located its insertion point via `(sheet_instances`, which exists only in the root .kicad_sch. Adding a component to any hierarchical sub-sheet raised "Could not find insertion point in schematic" and aborted the call. Fall back to inserting just before the closing paren of the outer (kicad_sch ...) form when the marker is absent. WireManager.add_label already uses the same fallback for hierarchical sub-sheets. Adds three regression tests in TestCreateComponentInstanceSubSheet covering successful insertion, paren balance, and sexpdata round-trip on a sub-sheet without (sheet_instances). --- python/commands/dynamic_symbol_loader.py | 13 +++- tests/test_add_schematic_component.py | 97 ++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 3 deletions(-) diff --git a/python/commands/dynamic_symbol_loader.py b/python/commands/dynamic_symbol_loader.py index 583d749..a81dacf 100644 --- a/python/commands/dynamic_symbol_loader.py +++ b/python/commands/dynamic_symbol_loader.py @@ -483,9 +483,16 @@ class DynamicSymbolLoader: insert_marker = "(sheet_instances" insert_at = content.rfind(insert_marker) if insert_at == -1: - raise ValueError("Could not find insertion point in schematic") - - content = content[:insert_at] + instance_block + "\n " + content[insert_at:] + # Hierarchical sub-sheets don't carry (sheet_instances ...) — only the + # root .kicad_sch does. Fall back to inserting just before the final + # closing paren of the outer (kicad_sch ...) form. + stripped = content.rstrip() + if not stripped.endswith(")"): + raise ValueError("Could not find insertion point in schematic") + insert_at = len(stripped) - 1 + content = content[:insert_at] + instance_block + "\n" + content[insert_at:] + else: + content = content[:insert_at] + instance_block + "\n " + content[insert_at:] with open(schematic_path, "w", encoding="utf-8") as f: f.write(content) diff --git a/tests/test_add_schematic_component.py b/tests/test_add_schematic_component.py index 92f1d1f..2094051 100644 --- a/tests/test_add_schematic_component.py +++ b/tests/test_add_schematic_component.py @@ -196,6 +196,103 @@ class TestHandlerAddSchematicComponent: assert 2 in units +# --------------------------------------------------------------------------- +# Hierarchical sub-sheets — no (sheet_instances ...) block +# --------------------------------------------------------------------------- + + +# Minimal sub-sheet: same outer (kicad_sch ...) form as a root schematic but +# WITHOUT (sheet_instances ...). Hierarchical KiCad designs only carry that +# block in the root .kicad_sch — every child sheet ends after lib_symbols / +# any placed (symbol ...) blocks. The fix under test must insert new symbol +# instances before the closing paren of (kicad_sch ...) when the marker is +# missing. +SUB_SHEET_NO_SHEET_INSTANCES = """(kicad_sch +\t(version 20260306) +\t(generator "eeschema") +\t(generator_version "10.0") +\t(uuid "bbbb2222-2222-2222-2222-bbbbbbbbbbbb") +\t(paper "A4") +\t(lib_symbols) +) +""" + + +@pytest.mark.unit +class TestCreateComponentInstanceSubSheet: + """Hierarchical sub-sheets don't have (sheet_instances ...). + + Before the fix, create_component_instance raised + 'Could not find insertion point in schematic' on any sub-sheet, blocking + every add_schematic_component call into a hierarchical design's child + sheet. + """ + + def setup_method(self) -> None: + from commands.dynamic_symbol_loader import DynamicSymbolLoader + + self.DynamicSymbolLoader = DynamicSymbolLoader + + def _loader(self) -> Any: + return self.DynamicSymbolLoader() + + def test_sub_sheet_insertion_succeeds(self, tmp_path: Any) -> None: + sch = tmp_path / "child.kicad_sch" + sch.write_text(SUB_SHEET_NO_SHEET_INSTANCES, encoding="utf-8") + + ok = self._loader().create_component_instance( + sch, "Device", "R", reference="R_TEST", value="100k", x=50, y=50 + ) + + assert ok is True + content = sch.read_text(encoding="utf-8") + assert '"R_TEST"' in content + assert "100k" in content + + def test_sub_sheet_keeps_outer_form_balanced(self, tmp_path: Any) -> None: + """The new symbol must land inside (kicad_sch ...), with parens balanced.""" + sch = tmp_path / "child.kicad_sch" + sch.write_text(SUB_SHEET_NO_SHEET_INSTANCES, encoding="utf-8") + + self._loader().create_component_instance( + sch, "Device", "R", reference="R_TEST", value="1k", x=10, y=10 + ) + + content = sch.read_text(encoding="utf-8") + assert content.count("(") == content.count( + ")" + ), "Inserting into a sub-sheet must keep parens balanced" + # The outer form must still parse via sexpdata. + import sexpdata + + parsed = sexpdata.loads(content) + assert isinstance(parsed, list) + assert parsed[0] == sexpdata.Symbol("kicad_sch") + + def test_sub_sheet_round_trips_via_sexpdata(self, tmp_path: Any) -> None: + """The injected symbol must survive a sexpdata load+dump round-trip.""" + import sexpdata + + sch = tmp_path / "child.kicad_sch" + sch.write_text(SUB_SHEET_NO_SHEET_INSTANCES, encoding="utf-8") + + self._loader().create_component_instance( + sch, "Device", "R", reference="R_TEST", value="1k", x=10, y=10 + ) + + parsed = sexpdata.loads(sch.read_text(encoding="utf-8")) + # The placed (symbol (lib_id ...) ...) block must be a top-level child of kicad_sch. + symbol_items = [ + item + for item in parsed[1:] + if isinstance(item, list) and len(item) > 0 and item[0] == sexpdata.Symbol("symbol") + ] + # Confirm at least one of those carries our reference. + assert any( + sexpdata.dumps(s).find('"R_TEST"') >= 0 for s in symbol_items + ), "Reference 'R_TEST' should appear in a top-level (symbol ...) child" + + # --------------------------------------------------------------------------- # Mirror parameter — known gap # ---------------------------------------------------------------------------