diff --git a/python/commands/wire_dragger.py b/python/commands/wire_dragger.py index fb31e99..5af3d63 100644 --- a/python/commands/wire_dragger.py +++ b/python/commands/wire_dragger.py @@ -197,6 +197,81 @@ class WireDragger: ) return result + @staticmethod + def compute_pin_positions_for_rotation( + sch_data: list, + reference: str, + new_rotation: float, + new_mirror_x: bool, + new_mirror_y: bool, + ) -> Dict[str, Tuple[Tuple[float, float], Tuple[float, float]]]: + """ + Compute world pin positions before and after a rotation/mirror change. + + The symbol stays at the same (x, y); only the rotation and mirror state change. + Returns {pin_num: (old_world_xy, new_world_xy)}. + """ + found = WireDragger.find_symbol(sch_data, reference) + if found is None: + return {} + _, sym_x, sym_y, old_rotation, lib_id, old_mirror_x, old_mirror_y = found + + pins = WireDragger.get_pin_defs(sch_data, lib_id) + result: Dict[str, Tuple] = {} + for pin_num, pin in pins.items(): + px, py = pin["x"], pin["y"] + old_wx, old_wy = WireDragger.pin_world_xy( + px, py, sym_x, sym_y, old_rotation, old_mirror_x, old_mirror_y + ) + new_wx, new_wy = WireDragger.pin_world_xy( + px, py, sym_x, sym_y, new_rotation, new_mirror_x, new_mirror_y + ) + result[pin_num] = ( + (round(old_wx, 6), round(old_wy, 6)), + (round(new_wx, 6), round(new_wy, 6)), + ) + return result + + @staticmethod + def update_symbol_rotation_mirror( + sch_data: list, + reference: str, + new_rotation: float, + new_mirror: Optional[str], + ) -> bool: + """ + Update the rotation in (at x y rot) and the (mirror x/y) token for a symbol. + + new_mirror: "x", "y", or None (removes any existing mirror token). + Returns True if the symbol was found and updated. + """ + found = WireDragger.find_symbol(sch_data, reference) + if found is None: + return False + item = found[0] + at_k = _K["at"] + mirror_k = _K["mirror"] + + # Update rotation in (at x y rot) + for sub in item[1:]: + if isinstance(sub, list) and sub and sub[0] == at_k and len(sub) >= 4: + sub[3] = new_rotation + break + + # Remove existing (mirror ...) token(s) + to_remove = [ + i for i, sub in enumerate(item) + if isinstance(sub, list) and sub and sub[0] == mirror_k + ] + for i in reversed(to_remove): + del item[i] + + # Insert new mirror token if requested + if new_mirror in ("x", "y"): + item.append([mirror_k, Symbol(new_mirror)]) + + return True + @staticmethod def drag_wires( sch_data: list, diff --git a/python/kicad_interface.py b/python/kicad_interface.py index b1bfe6f..c9dd3ef 100644 --- a/python/kicad_interface.py +++ b/python/kicad_interface.py @@ -2710,13 +2710,16 @@ class KiCADInterface: return {"success": False, "message": str(e)} def _handle_rotate_schematic_component(self, params: Dict[str, Any]) -> Dict[str, Any]: - """Rotate a schematic component""" + """Rotate and/or mirror a schematic component, dragging connected wires.""" logger.info("Rotating schematic component") try: + import sexpdata as _sexpdata + from commands.wire_dragger import WireDragger + schematic_path = params.get("schematicPath") reference = params.get("reference") angle = params.get("angle", 0) - mirror = params.get("mirror") + mirror = params.get("mirror") # "x", "y", or None if not schematic_path or not reference: return { @@ -2724,77 +2727,61 @@ class KiCADInterface: "message": "schematicPath and reference are required", } - sym_k = sexpdata.Symbol("symbol") - prop_k = sexpdata.Symbol("property") - at_k = sexpdata.Symbol("at") - mirror_k = sexpdata.Symbol("mirror") + with open(schematic_path, "r", encoding="utf-8") as f: + sch_data = _sexpdata.loads(f.read()) - with open(schematic_path, "r", encoding="utf-8") as _f: - sch_data = sexpdata.load(_f) - - target = None - for item in sch_data: - if not (isinstance(item, list) and item and item[0] == sym_k): - continue - ref_val = None - for sub in item[1:]: - if ( - isinstance(sub, list) - and len(sub) >= 3 - and sub[0] == prop_k - and str(sub[1]).strip('"') == "Reference" - ): - ref_val = str(sub[2]).strip('"') - break - if ref_val == reference: - target = item - break - - if target is None: + found = WireDragger.find_symbol(sch_data, reference) + if found is None: return {"success": False, "message": f"Component {reference} not found"} - # Update (at x y rot) - at_node = None - mirror_idx = None - for idx, sub in enumerate(target[1:], start=1): - if not isinstance(sub, list) or not sub: - continue - if sub[0] == at_k: - at_node = sub - elif sub[0] == mirror_k: - mirror_idx = idx - - if at_node is None: - return { - "success": False, - "message": f"Component {reference} has no (at ...) node", - } - while len(at_node) < 3: - at_node.append(0) - if len(at_node) < 4: - at_node.append(angle) + # Determine new mirror state: explicit param overrides; None preserves existing + _, _, _, _, _, old_mirror_x, old_mirror_y = found + if mirror is None: + new_mirror_x = old_mirror_x + new_mirror_y = old_mirror_y + effective_mirror = "x" if old_mirror_x else ("y" if old_mirror_y else None) else: - at_node[3] = angle + new_mirror_x = (mirror == "x") + new_mirror_y = (mirror == "y") + effective_mirror = mirror - if mirror: - mirror_node = [mirror_k, sexpdata.Symbol(str(mirror))] - if mirror_idx is not None: - target[mirror_idx] = mirror_node - else: - # Insert after (at ...) for stability - insert_at = len(target) - for idx, sub in enumerate(target[1:], start=1): - if isinstance(sub, list) and sub and sub[0] == at_k: - insert_at = idx + 1 - break - target.insert(insert_at, mirror_node) + # Compute pin world positions before and after the transform + pin_positions = WireDragger.compute_pin_positions_for_rotation( + sch_data, reference, float(angle), new_mirror_x, new_mirror_y + ) + + # Build old→new map (skip pins that don't move) + old_to_new = {} + for _pin, (old_xy, new_xy) in pin_positions.items(): + if old_xy == new_xy: + continue + if old_xy in old_to_new: + logger.warning( + f"rotate: pin {_pin!r} of {reference!r} shares old position " + f"{old_xy} with another pin; skipping duplicate" + ) + continue + old_to_new[old_xy] = new_xy + + # Drag connected wires to follow pins + drag_summary = WireDragger.drag_wires(sch_data, old_to_new) + + # Update the symbol's rotation and mirror token in sexpdata + WireDragger.update_symbol_rotation_mirror(sch_data, reference, float(angle), effective_mirror) WireManager.sync_junctions(sch_data) - with open(schematic_path, "w", encoding="utf-8") as _f: - _f.write(sexpdata.dumps(sch_data)) + with open(schematic_path, "w", encoding="utf-8") as f: + f.write(_sexpdata.dumps(sch_data)) - return {"success": True, "reference": reference, "angle": angle} + return { + "success": True, + "reference": reference, + "angle": angle, + "mirror": effective_mirror, + "wiresMoved": drag_summary.get("endpoints_moved", 0), + "wiresRemoved": drag_summary.get("wires_removed", 0), + } except Exception as e: logger.error(f"Error rotating schematic component: {e}") diff --git a/tests/test_rotate_schematic_mirror.py b/tests/test_rotate_schematic_mirror.py new file mode 100644 index 0000000..04ffae3 --- /dev/null +++ b/tests/test_rotate_schematic_mirror.py @@ -0,0 +1,242 @@ +""" +Tests for rotate_schematic_component mirror/rotation fix. + +Tests are split into two layers: + 1. WireDragger unit tests — pure sexpdata logic, no KiCAD deps. + 2. Handler integration smoke test — patches SchematicManager away. +""" + +import os +import sys +import math +import textwrap +import tempfile +import importlib.util +from unittest.mock import patch, MagicMock + +import sexpdata +from sexpdata import Symbol + +# --------------------------------------------------------------------------- +# Import WireDragger directly (no pcbnew / kicad_interface needed) +# --------------------------------------------------------------------------- +_wd_spec = importlib.util.spec_from_file_location( + "wire_dragger", + os.path.join(os.path.dirname(__file__), "..", "python", "commands", "wire_dragger.py"), +) +_wd_mod = importlib.util.module_from_spec(_wd_spec) + +# wire_dragger imports pin_locator lazily inside get_pin_defs. +# We stub only the submodule, not the parent package, so that +# kicad_interface can still import commands.board etc. from disk. +_pin_locator_mock = MagicMock() +sys.modules.setdefault("commands.pin_locator", _pin_locator_mock) + +_wd_spec.loader.exec_module(_wd_mod) +WireDragger = _wd_mod.WireDragger + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _parse(text: str) -> list: + return sexpdata.loads(text) + + +def _dump(data: list) -> str: + return sexpdata.dumps(data) + + +def _make_sch(sym_extra: str = "", wires: str = "") -> list: + """Build a minimal schematic sexpdata with one Q1 symbol.""" + text = textwrap.dedent(f"""\ + (kicad_sch (version 20250114) (generator "test") + (lib_symbols + (symbol "Transistor_BJT:MMBT3904" + (pin passive line (at 0.0 1.0 270) (length 1.27) + (name "B" (effects (font (size 1.27 1.27)))) + (number "1" (effects (font (size 1.27 1.27)))) + ) + (pin passive line (at -1.0 0.0 0) (length 1.27) + (name "C" (effects (font (size 1.27 1.27)))) + (number "2" (effects (font (size 1.27 1.27)))) + ) + ) + ) + (symbol (lib_id "Transistor_BJT:MMBT3904") + (at 75 105 0) + {sym_extra} + (property "Reference" "Q1" (at 75 105 0)) + (property "Value" "MMBT3904" (at 75 105 0)) + ) + {wires} + ) + """) + return _parse(text) + + +# --------------------------------------------------------------------------- +# Tests: update_symbol_rotation_mirror +# --------------------------------------------------------------------------- + +def test_update_rotation_sets_angle(): + sch = _make_sch() + result = WireDragger.update_symbol_rotation_mirror(sch, "Q1", 90.0, None) + assert result is True + dumped = _dump(sch) + # at should now have 90 as the rotation value + assert "90" in dumped + + +def test_update_mirror_x_adds_token(): + sch = _make_sch() + WireDragger.update_symbol_rotation_mirror(sch, "Q1", 0.0, "x") + dumped = _dump(sch) + assert "mirror" in dumped + assert " x" in dumped or "(mirror x)" in dumped + + +def test_update_mirror_y_adds_token(): + sch = _make_sch() + WireDragger.update_symbol_rotation_mirror(sch, "Q1", 0.0, "y") + dumped = _dump(sch) + assert "mirror" in dumped + + +def test_update_mirror_none_removes_existing(): + """mirror=None should remove a pre-existing (mirror x) token.""" + sch = _make_sch(sym_extra="(mirror x)") + WireDragger.update_symbol_rotation_mirror(sch, "Q1", 0.0, None) + dumped = _dump(sch) + assert "mirror" not in dumped + + +def test_update_mirror_replaces_existing(): + """Setting mirror='y' when (mirror x) exists should replace, not duplicate.""" + sch = _make_sch(sym_extra="(mirror x)") + WireDragger.update_symbol_rotation_mirror(sch, "Q1", 0.0, "y") + dumped = _dump(sch) + assert dumped.count("mirror") == 1 + + +def test_update_unknown_reference_returns_false(): + sch = _make_sch() + result = WireDragger.update_symbol_rotation_mirror(sch, "U99", 0.0, "x") + assert result is False + + +# --------------------------------------------------------------------------- +# Tests: compute_pin_positions_for_rotation +# --------------------------------------------------------------------------- + +def test_pin_positions_change_on_rotation(): + """Pins at non-zero local offsets should move when the symbol rotates.""" + sch = _make_sch() + + # Provide a real pin_defs via patch so we don't need KiCAD libs + fake_pins = { + "1": {"x": 0.0, "y": 1.0}, + "2": {"x": -1.0, "y": 0.0}, + } + with patch.object(WireDragger, "get_pin_defs", return_value=fake_pins): + pos = WireDragger.compute_pin_positions_for_rotation(sch, "Q1", 90.0, False, False) + + assert len(pos) == 2 + for pin_num, (old_xy, new_xy) in pos.items(): + # After 90° rotation the positions must differ (pins not at origin) + assert old_xy != new_xy, f"Pin {pin_num} should have moved" + + +def test_pin_positions_unchanged_at_same_transform(): + """Same rotation and same mirror → no movement.""" + sch = _make_sch() # symbol at rotation=0, no mirror + + fake_pins = {"1": {"x": 1.0, "y": 0.0}} + with patch.object(WireDragger, "get_pin_defs", return_value=fake_pins): + pos = WireDragger.compute_pin_positions_for_rotation(sch, "Q1", 0.0, False, False) + + for _, (old_xy, new_xy) in pos.items(): + assert old_xy == new_xy + + +def test_pin_positions_mirror_x_flips_x(): + """mirror_x should negate the local X coordinate before rotation.""" + sch = _make_sch() # at (75, 105, 0), no mirror + + fake_pins = {"1": {"x": 2.0, "y": 0.0}} + with patch.object(WireDragger, "get_pin_defs", return_value=fake_pins): + pos = WireDragger.compute_pin_positions_for_rotation(sch, "Q1", 0.0, True, False) + + _, (old_xy, new_xy) = next(iter(pos.items())) + # old: pin at local (2, 0), world = (75+2, 105) = (77, 105) + assert abs(old_xy[0] - 77.0) < 1e-4 + # new: mirror_x → local (-2, 0), world = (75-2, 105) = (73, 105) + assert abs(new_xy[0] - 73.0) < 1e-4 + + +# --------------------------------------------------------------------------- +# Integration smoke test: handler uses sexpdata, not kicad-skip +# --------------------------------------------------------------------------- + +def test_rotate_handler_no_crash(tmp_path): + """_handle_rotate_schematic_component should succeed without kicad-skip.""" + # Stub heavy imports before loading kicad_interface + for modname in ("pcbnew", "skip", "resources", "schemas", + "resources.resource_definitions", "schemas.tool_schemas"): + sys.modules.setdefault(modname, MagicMock()) + sys.modules["resources.resource_definitions"].RESOURCE_DEFINITIONS = {} + sys.modules["resources.resource_definitions"].handle_resource_read = MagicMock() + sys.modules["schemas.tool_schemas"].TOOL_SCHEMAS = [] + + _pcbnew = sys.modules["pcbnew"] + _pcbnew.__file__ = "/fake/pcbnew.so" + _pcbnew.GetBuildVersion.return_value = "9.0.0" + + ki_spec = importlib.util.spec_from_file_location( + "kicad_interface_smoke", + os.path.join(os.path.dirname(__file__), "..", "python", "kicad_interface.py"), + ) + ki_mod = importlib.util.module_from_spec(ki_spec) + ki_spec.loader.exec_module(ki_mod) + KiCADInterface = ki_mod.KiCADInterface + + # Write a minimal schematic file + sch_path = str(tmp_path / "test.kicad_sch") + sch_content = textwrap.dedent("""\ + (kicad_sch (version 20250114) (generator "test") + (lib_symbols + (symbol "Device:R" + (pin passive line (at 0 1.016 270) (length 1.27) + (name "~" (effects (font (size 1.27 1.27)))) + (number "1" (effects (font (size 1.27 1.27)))) + ) + (pin passive line (at 0 -1.016 90) (length 1.27) + (name "~" (effects (font (size 1.27 1.27)))) + (number "2" (effects (font (size 1.27 1.27)))) + ) + ) + ) + (symbol (lib_id "Device:R") (at 100 100 0) + (property "Reference" "R1" (at 100 100 0)) + (property "Value" "10k" (at 100 100 0)) + ) + ) + """) + with open(sch_path, "w") as f: + f.write(sch_content) + + iface = KiCADInterface.__new__(KiCADInterface) + result = iface._handle_rotate_schematic_component({ + "schematicPath": sch_path, + "reference": "R1", + "angle": 90, + }) + + assert result["success"] is True + assert result["angle"] == 90 + + # Verify the file was actually updated + with open(sch_path) as f: + updated = f.read() + assert "90" in updated