diff --git a/python/commands/dynamic_symbol_loader.py b/python/commands/dynamic_symbol_loader.py index 4c9b778..22ea7c4 100644 --- a/python/commands/dynamic_symbol_loader.py +++ b/python/commands/dynamic_symbol_loader.py @@ -425,6 +425,14 @@ class DynamicSymbolLoader: (property "Datasheet" "~" (at {x} {y} 0) (effects (font (size 1.27 1.27)) (hide yes)) ) + (instances + (project "project" + (path "/" + (reference "{reference}") + (unit 1) + ) + ) + ) )""" with open(schematic_path, "r", encoding="utf-8") as f: diff --git a/python/commands/wire_dragger.py b/python/commands/wire_dragger.py new file mode 100644 index 0000000..2ad7a54 --- /dev/null +++ b/python/commands/wire_dragger.py @@ -0,0 +1,439 @@ +""" +WireDragger — drag connected wires when a schematic component is moved. + +All methods operate on in-memory sexpdata lists (no disk I/O). +""" + +import logging +import math +import uuid +from typing import Dict, List, Optional, Tuple + +import sexpdata +from sexpdata import Symbol + +logger = logging.getLogger("kicad_interface") + +# Module-level Symbol constants +_K = { + name: Symbol(name) + for name in [ + "symbol", + "at", + "lib_id", + "mirror", + "lib_symbols", + "pts", + "xy", + "wire", + "junction", + "property", + "stroke", + "width", + "type", + "uuid", + ] +} + +EPS = 1e-4 # mm — coordinate match tolerance + + +def _rotate(x: float, y: float, angle_deg: float) -> Tuple[float, float]: + """Rotate (x, y) around the origin by angle_deg degrees (CCW).""" + if angle_deg == 0: + return x, y + rad = math.radians(angle_deg) + c, s = math.cos(rad), math.sin(rad) + return x * c - y * s, x * s + y * c + + +def _coords_match(ax: float, ay: float, bx: float, by: float, eps: float = EPS) -> bool: + return abs(ax - bx) < eps and abs(ay - by) < eps + + +class WireDragger: + """Pure-logic helpers for wire-endpoint dragging during component moves.""" + + @staticmethod + def find_symbol(sch_data: list, reference: str): + """ + Find a placed symbol by reference designator. + + Returns (symbol_item, old_x, old_y, rotation, lib_id, mirror_x, mirror_y) + or None if the reference is not found. + + mirror_x=True means the symbol has (mirror x) — flips the X local axis. + mirror_y=True means the symbol has (mirror y) — flips the Y local axis. + """ + sym_k = _K["symbol"] + prop_k = _K["property"] + at_k = _K["at"] + lib_id_k = _K["lib_id"] + mirror_k = _K["mirror"] + + for item in sch_data: + if not (isinstance(item, list) and item and item[0] == sym_k): + continue + + # Check Reference property + ref_val = None + for sub in item[1:]: + if isinstance(sub, list) and len(sub) >= 3 and sub[0] == prop_k: + if str(sub[1]).strip('"') == "Reference": + ref_val = str(sub[2]).strip('"') + break + if ref_val != reference: + continue + + old_x = old_y = rotation = 0.0 + lib_id = "" + mirror_x = mirror_y = False + + for sub in item[1:]: + if not isinstance(sub, list) or not sub: + continue + tag = sub[0] + if tag == at_k: + if len(sub) >= 3: + old_x = float(sub[1]) + old_y = float(sub[2]) + if len(sub) >= 4: + rotation = float(sub[3]) + elif tag == lib_id_k and len(sub) >= 2: + lib_id = str(sub[1]).strip('"') + elif tag == mirror_k and len(sub) >= 2: + mv = str(sub[1]) + if mv == "x": + mirror_x = True + elif mv == "y": + mirror_y = True + + return item, old_x, old_y, rotation, lib_id, mirror_x, mirror_y + + return None + + @staticmethod + def get_pin_defs(sch_data: list, lib_id: str) -> Dict: + """ + Get pin definitions from lib_symbols for the given lib_id. + + Returns the same dict format as PinLocator.parse_symbol_definition: + {pin_num: {"x": ..., "y": ..., ...}}. + """ + from commands.pin_locator import PinLocator + + lib_sym_k = _K["lib_symbols"] + symbol_k = _K["symbol"] + + for item in sch_data: + if not (isinstance(item, list) and item and item[0] == lib_sym_k): + continue + for sym_def in item[1:]: + if not (isinstance(sym_def, list) and sym_def and sym_def[0] == symbol_k): + continue + if len(sym_def) < 2: + continue + name = str(sym_def[1]).strip('"') + if name == lib_id: + return PinLocator.parse_symbol_definition(sym_def) + break # only one lib_symbols section + return {} + + @staticmethod + def pin_world_xy( + px: float, + py: float, + sym_x: float, + sym_y: float, + rotation: float, + mirror_x: bool, + mirror_y: bool, + ) -> Tuple[float, float]: + """ + Compute the world coordinate of a pin given the symbol transform. + + KiCAD applies mirror first (in local space), then rotation, then translation. + mirror_x negates the local X axis; mirror_y negates the local Y axis. + """ + lx, ly = px, py + if mirror_x: + lx = -lx + if mirror_y: + ly = -ly + rx, ry = _rotate(lx, ly, rotation) + return sym_x + rx, sym_y + ry + + @staticmethod + def compute_pin_positions( + sch_data: list, + reference: str, + new_x: float, + new_y: float, + ) -> Dict[str, Tuple[Tuple[float, float], Tuple[float, float]]]: + """ + Compute world pin positions before and after a component move. + + Returns {pin_num: (old_world_xy, new_world_xy)}. + old_world_xy uses the symbol's current position; new_world_xy uses (new_x, new_y). + """ + found = WireDragger.find_symbol(sch_data, reference) + if found is None: + return {} + _, old_x, old_y, rotation, lib_id, mirror_x, 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, old_x, old_y, rotation, mirror_x, mirror_y + ) + new_wx, new_wy = WireDragger.pin_world_xy( + px, py, new_x, new_y, rotation, mirror_x, 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 drag_wires( + sch_data: list, + old_to_new: Dict[Tuple[float, float], Tuple[float, float]], + eps: float = EPS, + ) -> Dict: + """ + Move wire endpoints and junctions from old positions to new positions. + Removes zero-length wires that result from the move. + Modifies sch_data in place. + + old_to_new: {(old_x, old_y): (new_x, new_y)} + + Returns {'endpoints_moved': N, 'wires_removed': M}. + """ + wire_k = _K["wire"] + pts_k = _K["pts"] + xy_k = _K["xy"] + junction_k = _K["junction"] + at_k = _K["at"] + + def find_new(x: float, y: float): + for (ox, oy), (nx, ny) in old_to_new.items(): + if _coords_match(x, y, ox, oy, eps): + return nx, ny + return None + + endpoints_moved = 0 + zero_length_indices = [] + + # First pass: update wire endpoints + for idx, item in enumerate(sch_data): + if not (isinstance(item, list) and item and item[0] == wire_k): + continue + + pts_sub = None + for sub in item[1:]: + if isinstance(sub, list) and sub and sub[0] == pts_k: + pts_sub = sub + break + if pts_sub is None: + continue + + xy_items = [ + p for p in pts_sub[1:] if isinstance(p, list) and len(p) >= 3 and p[0] == xy_k + ] + for xy_item in xy_items: + nc = find_new(float(xy_item[1]), float(xy_item[2])) + if nc is not None: + xy_item[1] = nc[0] + xy_item[2] = nc[1] + endpoints_moved += 1 + + # Check if this wire is now zero-length + if len(xy_items) >= 2: + x1, y1 = float(xy_items[0][1]), float(xy_items[0][2]) + x2, y2 = float(xy_items[-1][1]), float(xy_items[-1][2]) + if _coords_match(x1, y1, x2, y2, eps): + zero_length_indices.append(idx) + + # Remove zero-length wires (backwards to preserve indices) + for idx in reversed(zero_length_indices): + del sch_data[idx] + + # Second pass: update junctions + for item in sch_data: + if not (isinstance(item, list) and item and item[0] == junction_k): + continue + for sub in item[1:]: + if isinstance(sub, list) and sub and sub[0] == at_k and len(sub) >= 3: + nc = find_new(float(sub[1]), float(sub[2])) + if nc is not None: + sub[1] = nc[0] + sub[2] = nc[1] + break + + return { + "endpoints_moved": endpoints_moved, + "wires_removed": len(zero_length_indices), + } + + @staticmethod + def update_symbol_position(sch_data: list, reference: str, new_x: float, new_y: float) -> bool: + """ + Update the (at x y rot) of the named symbol in sch_data. + 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"] + prop_k = _K["property"] + + # Find current position and compute delta + old_x = old_y = None + for sub in item[1:]: + if isinstance(sub, list) and sub and sub[0] == at_k and len(sub) >= 3: + old_x, old_y = sub[1], sub[2] + sub[1] = new_x + sub[2] = new_y + break + if old_x is None or old_y is None: + return False + + dx = new_x - old_x + dy = new_y - old_y + + # Shift all property label positions by the same delta + for sub in item[1:]: + if isinstance(sub, list) and sub and sub[0] == prop_k: + for psub in sub[1:]: + if isinstance(psub, list) and psub and psub[0] == at_k and len(psub) >= 3: + psub[1] += dx + psub[2] += dy + break + return True + + @staticmethod + def _make_wire_sexp(x1: float, y1: float, x2: float, y2: float) -> list: + """Build a wire s-expression list in KiCAD schematic format.""" + wire_uuid = str(uuid.uuid4()) + return [ + _K["wire"], + [_K["pts"], [_K["xy"], x1, y1], [_K["xy"], x2, y2]], + [_K["stroke"], [_K["width"], 0], [_K["type"], Symbol("default")]], + [_K["uuid"], wire_uuid], + ] + + @staticmethod + def get_all_stationary_pin_positions( + sch_data: list, + moved_reference: str, + ) -> Dict[Tuple[float, float], str]: + """ + Return a map of {world_xy: reference} for every pin of every symbol + in sch_data *except* moved_reference. + + This is used to detect pins of stationary components that coincide + with pins of the moved component (touching-pin connections). + """ + sym_k = _K["symbol"] + prop_k = _K["property"] + result: Dict[Tuple[float, float], str] = {} + + for item in sch_data: + if not (isinstance(item, list) and item and item[0] == sym_k): + continue + # Determine reference + ref_val = None + for sub in item[1:]: + if isinstance(sub, list) and len(sub) >= 3 and sub[0] == prop_k: + if str(sub[1]).strip('"') == "Reference": + ref_val = str(sub[2]).strip('"') + break + if ref_val is None or ref_val == moved_reference: + continue + # Skip template / power symbols whose references start with special chars + # but we still want to handle them — no filtering needed here. + + # Find lib_id and position for this symbol + found = WireDragger.find_symbol(sch_data, ref_val) + if found is None: + continue + _, sx, sy, rotation, lib_id, mirror_x, mirror_y = found + pins = WireDragger.get_pin_defs(sch_data, lib_id) + for pin_num, pin in pins.items(): + wx, wy = WireDragger.pin_world_xy( + pin["x"], pin["y"], sx, sy, rotation, mirror_x, mirror_y + ) + key = (round(wx, 6), round(wy, 6)) + result[key] = ref_val + + return result + + @staticmethod + def synthesize_touching_pin_wires( + sch_data: list, + moved_reference: str, + pin_positions: Dict[str, Tuple[Tuple[float, float], Tuple[float, float]]], + eps: float = EPS, + ) -> int: + """ + Detect touching-pin connections and synthesize wire segments to bridge gaps + created by moving a component. + + For each pin of *moved_reference* whose old world position coincides with + a pin of a stationary component: + - If the pin moved (old_xy != new_xy), insert a wire from old_xy to new_xy. + - If the pin now lands on another stationary pin's position, skip (they touch again). + - If old_xy == new_xy, do nothing (no gap was created). + + Modifies sch_data in place. + Returns the number of wire segments synthesized. + """ + if not pin_positions: + return 0 + + stationary_pins = WireDragger.get_all_stationary_pin_positions(sch_data, moved_reference) + if not stationary_pins: + return 0 + + synthesized = 0 + + for pin_num, (old_xy, new_xy) in pin_positions.items(): + # Check if a stationary pin touches this pin's old position + touching = any( + _coords_match(old_xy[0], old_xy[1], sx, sy, eps) for (sx, sy) in stationary_pins + ) + if not touching: + continue + + # The pin has moved — check if it actually separated + if _coords_match(old_xy[0], old_xy[1], new_xy[0], new_xy[1], eps): + # Pin didn't actually move; no gap + continue + + # Check if the pin's new position happens to touch another stationary pin + # (component moved into a different touching position — no wire needed) + rejoining = any( + _coords_match(new_xy[0], new_xy[1], sx, sy, eps) for (sx, sy) in stationary_pins + ) + if rejoining: + logger.debug( + f"Pin {moved_reference}/{pin_num} moved from {old_xy} to {new_xy} " + f"and rejoins another stationary pin; no wire synthesized" + ) + continue + + logger.info( + f"Synthesizing wire for touching-pin connection: " + f"{moved_reference}/{pin_num} moved from {old_xy} to {new_xy}" + ) + wire = WireDragger._make_wire_sexp(old_xy[0], old_xy[1], new_xy[0], new_xy[1]) + # Insert before the last item (sheet_instances) to keep file tidy, + # but appending is also valid — just append. + sch_data.append(wire) + synthesized += 1 + + return synthesized diff --git a/python/kicad_interface.py b/python/kicad_interface.py index b3903ad..1c14710 100644 --- a/python/kicad_interface.py +++ b/python/kicad_interface.py @@ -1283,6 +1283,46 @@ class KiCADInterface: logger.error(f"Error listing schematic libraries: {str(e)}") return {"success": False, "message": str(e)} + def _handle_find_unconnected_pins(self, params): + """List component pins with no wire/label/power symbol touching them""" + logger.info("Finding unconnected pins") + try: + from commands.schematic_analysis import find_unconnected_pins + + schematic_path = params.get("schematicPath") + if not schematic_path: + return {"success": False, "message": "schematicPath is required"} + result = find_unconnected_pins(schematic_path) + return {"success": True, **result} + except ImportError: + return { + "success": False, + "message": "schematic_analysis module not available", + } + except Exception as e: + logger.error(f"Error finding unconnected pins: {e}") + return {"success": False, "message": str(e)} + + def _handle_check_wire_collisions(self, params): + """Detect wires passing through component bodies without connecting to pins""" + logger.info("Checking wire collisions") + try: + from commands.schematic_analysis import check_wire_collisions + + schematic_path = params.get("schematicPath") + if not schematic_path: + return {"success": False, "message": "schematicPath is required"} + result = check_wire_collisions(schematic_path) + return {"success": True, **result} + except ImportError: + return { + "success": False, + "message": "schematic_analysis module not available", + } + except Exception as e: + logger.error(f"Error checking wire collisions: {e}") + return {"success": False, "message": str(e)} + # ------------------------------------------------------------------ # # Footprint handlers # # ------------------------------------------------------------------ # @@ -1998,14 +2038,18 @@ class KiCADInterface: return {"success": False, "message": str(e)} def _handle_move_schematic_component(self, params): - """Move a schematic component to a new position""" + """Move a schematic component to a new position, dragging connected wires.""" logger.info("Moving schematic component") try: + import sexpdata as _sexpdata + from commands.wire_dragger import WireDragger + schematic_path = params.get("schematicPath") reference = params.get("reference") position = params.get("position", {}) new_x = position.get("x") new_y = position.get("y") + preserve_wires = params.get("preserveWires", True) if not schematic_path or not reference: return { @@ -2018,30 +2062,57 @@ class KiCADInterface: "message": "position with x and y is required", } - schematic = SchematicManager.load_schematic(schematic_path) - if not schematic: - return {"success": False, "message": "Failed to load schematic"} + with open(schematic_path, "r", encoding="utf-8") as f: + sch_data = _sexpdata.loads(f.read()) - # Find the symbol - for symbol in schematic.symbol: - if not hasattr(symbol.property, "Reference"): - continue - if symbol.property.Reference.value == reference: - old_pos = list(symbol.at.value) - old_position = {"x": float(old_pos[0]), "y": float(old_pos[1])} + # Find symbol and record old position + found = WireDragger.find_symbol(sch_data, reference) + if found is None: + return {"success": False, "message": f"Component {reference} not found"} + _, old_x, old_y = found[0], found[1], found[2] + old_position = {"x": old_x, "y": old_y} - # Preserve rotation (third element) - rotation = float(old_pos[2]) if len(old_pos) > 2 else 0 - symbol.at.value = [new_x, new_y, rotation] + drag_summary = {} + if preserve_wires: + # Compute pin world positions before and after the move + pin_positions = WireDragger.compute_pin_positions( + sch_data, reference, float(new_x), float(new_y) + ) + # Build old→new coordinate map (deduplicate coincident pins) + old_to_new = {} + for _pin, (old_xy, new_xy) in pin_positions.items(): + if old_xy in old_to_new: + logger.warning( + f"move_schematic_component: pin {_pin!r} of {reference!r} " + f"shares old position {old_xy} with another pin; " + f"keeping first entry, skipping duplicate" + ) + continue + old_to_new[old_xy] = new_xy - SchematicManager.save_schematic(schematic, schematic_path) - return { - "success": True, - "oldPosition": old_position, - "newPosition": {"x": new_x, "y": new_y}, - } + drag_summary = WireDragger.drag_wires(sch_data, old_to_new) - return {"success": False, "message": f"Component {reference} not found"} + # Synthesize wires for touching-pin connections after dragging, + # so drag_wires doesn't accidentally move and collapse the new wire. + wires_synthesized = WireDragger.synthesize_touching_pin_wires( + sch_data, reference, pin_positions + ) + drag_summary["wires_synthesized"] = wires_synthesized + + # Update symbol position + WireDragger.update_symbol_position(sch_data, reference, float(new_x), float(new_y)) + + with open(schematic_path, "w", encoding="utf-8") as f: + f.write(_sexpdata.dumps(sch_data)) + + return { + "success": True, + "oldPosition": old_position, + "newPosition": {"x": new_x, "y": new_y}, + "wiresMoved": drag_summary.get("endpoints_moved", 0), + "wiresRemoved": drag_summary.get("wires_removed", 0), + "wiresSynthesized": drag_summary.get("wires_synthesized", 0), + } except Exception as e: logger.error(f"Error moving schematic component: {e}") @@ -2074,21 +2145,18 @@ class KiCADInterface: continue if symbol.property.Reference.value == reference: pos = list(symbol.at.value) - pos[2] = angle if len(pos) > 2 else angle while len(pos) < 3: pos.append(0) pos[2] = angle symbol.at.value = pos - # Handle mirror if specified if mirror: if hasattr(symbol, "mirror"): symbol.mirror.value = mirror else: logger.warning( f"Mirror '{mirror}' requested for {reference}, " - f"but symbol does not have a 'mirror' attribute; " - f"mirror not applied" + f"but symbol has no mirror attribute; skipped" ) SchematicManager.save_schematic(schematic, schematic_path) @@ -2159,7 +2227,7 @@ class KiCADInterface: old_ref = symbol.property.Reference.value new_ref = f"{prefix}{next_num}" - symbol.property.Reference.value = new_ref + symbol.setAllReferences(new_ref) existing_refs[prefix].add(next_num) uuid_val = str(symbol.uuid.value) if hasattr(symbol, "uuid") else "" diff --git a/python/tests/test_move_with_wire_preservation.py b/python/tests/test_move_with_wire_preservation.py new file mode 100644 index 0000000..fceefc8 --- /dev/null +++ b/python/tests/test_move_with_wire_preservation.py @@ -0,0 +1,1009 @@ +""" +Tests for move_schematic_component with wire preservation (WireDragger). + +Unit tests use synthetic sexpdata lists — no disk I/O, no KiCAD install needed. +Integration tests copy empty.kicad_sch to a tempdir and exercise the full handler. +""" + +import shutil +import sys +import tempfile +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest +import sexpdata +from sexpdata import Symbol + +# Make python/ importable +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from commands.wire_dragger import EPS, WireDragger, _coords_match, _rotate + +TEMPLATE_PATH = Path(__file__).resolve().parent.parent / "templates" / "empty.kicad_sch" + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _sym(name: str) -> Symbol: + return Symbol(name) + + +def _make_wire(x1, y1, x2, y2): + return [ + _sym("wire"), + [_sym("pts"), [_sym("xy"), x1, y1], [_sym("xy"), x2, y2]], + [_sym("stroke"), [_sym("width"), 0], [_sym("type"), _sym("default")]], + [_sym("uuid"), "00000000-0000-0000-0000-000000000000"], + ] + + +def _make_junction(x, y): + return [ + _sym("junction"), + [_sym("at"), x, y], + [_sym("diameter"), 0], + [_sym("color"), 0, 0, 0, 0], + [_sym("uuid"), "00000000-0000-0000-0000-000000000001"], + ] + + +def _make_symbol(ref, x, y, rotation=0, lib_id="Device:R", mirror=None): + """Build a minimal placed-symbol s-expression.""" + item = [ + _sym("symbol"), + [_sym("lib_id"), lib_id], + [_sym("at"), x, y, rotation], + [_sym("unit"), 1], + [_sym("property"), "Reference", ref, [_sym("at"), x + 2, y, 0]], + [_sym("property"), "Value", "10k", [_sym("at"), x, y, 0]], + ] + if mirror: + item.append([_sym("mirror"), _sym(mirror)]) + return item + + +def _make_lib_symbol_r(): + """Minimal Device:R lib_symbols entry — pins at (0, 3.81) and (0, -3.81).""" + return [ + _sym("symbol"), + "Device:R", + [ + _sym("symbol"), + "R_1_1", + [ + _sym("pin"), + _sym("passive"), + _sym("line"), + [_sym("at"), 0, 3.81, 270], + [_sym("length"), 1.27], + [ + _sym("name"), + "~", + [_sym("effects"), [_sym("font"), [_sym("size"), 1.27, 1.27]]], + ], + [ + _sym("number"), + "1", + [_sym("effects"), [_sym("font"), [_sym("size"), 1.27, 1.27]]], + ], + ], + [ + _sym("pin"), + _sym("passive"), + _sym("line"), + [_sym("at"), 0, -3.81, 90], + [_sym("length"), 1.27], + [ + _sym("name"), + "~", + [_sym("effects"), [_sym("font"), [_sym("size"), 1.27, 1.27]]], + ], + [ + _sym("number"), + "2", + [_sym("effects"), [_sym("font"), [_sym("size"), 1.27, 1.27]]], + ], + ], + ], + ] + + +def _make_sch_data(extra_items=None): + """Build a minimal sch_data list with lib_symbols and sheet_instances.""" + data = [ + _sym("kicad_sch"), + [_sym("lib_symbols"), _make_lib_symbol_r()], + [_sym("sheet_instances"), [_sym("path"), "/", [_sym("page"), "1"]]], + ] + if extra_items: + # Insert before sheet_instances (last item) + for item in extra_items: + data.insert(len(data) - 1, item) + return data + + +# --------------------------------------------------------------------------- +# TestRotatePoint +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestRotatePoint: + def test_zero_rotation(self): + assert _rotate(1.0, 2.0, 0) == (1.0, 2.0) + + def test_90_degrees(self): + rx, ry = _rotate(1.0, 0.0, 90) + assert abs(rx - 0.0) < 1e-9 + assert abs(ry - 1.0) < 1e-9 + + def test_180_degrees(self): + rx, ry = _rotate(1.0, 0.0, 180) + assert abs(rx - (-1.0)) < 1e-9 + assert abs(ry - 0.0) < 1e-9 + + def test_270_degrees(self): + rx, ry = _rotate(0.0, 1.0, 270) + assert abs(rx - 1.0) < 1e-6 + assert abs(ry - 0.0) < 1e-6 + + +# --------------------------------------------------------------------------- +# TestFindSymbol +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestFindSymbol: + def test_returns_none_for_missing_reference(self): + sch = _make_sch_data([_make_symbol("R1", 10, 20)]) + assert WireDragger.find_symbol(sch, "R99") is None + + def test_returns_item_and_position(self): + sch = _make_sch_data([_make_symbol("R1", 10.5, 20.5, rotation=90)]) + result = WireDragger.find_symbol(sch, "R1") + assert result is not None + _, old_x, old_y, rotation, lib_id, mirror_x, mirror_y = result + assert abs(old_x - 10.5) < EPS + assert abs(old_y - 20.5) < EPS + assert abs(rotation - 90) < EPS + assert lib_id == "Device:R" + assert mirror_x is False + assert mirror_y is False + + def test_detects_mirror_x(self): + sch = _make_sch_data([_make_symbol("R1", 0, 0, mirror="x")]) + result = WireDragger.find_symbol(sch, "R1") + assert result is not None + assert result[5] is True # mirror_x + assert result[6] is False # mirror_y + + def test_detects_mirror_y(self): + sch = _make_sch_data([_make_symbol("R1", 0, 0, mirror="y")]) + result = WireDragger.find_symbol(sch, "R1") + assert result is not None + assert result[5] is False # mirror_x + assert result[6] is True # mirror_y + + +# --------------------------------------------------------------------------- +# TestComputePinPositions +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestComputePinPositions: + def test_resistor_at_origin_no_rotation(self): + """Device:R at (0, 0) rot=0 — pins at (0, 3.81) and (0, -3.81).""" + sch = _make_sch_data([_make_symbol("R1", 0, 0)]) + positions = WireDragger.compute_pin_positions(sch, "R1", 10, 20) + assert "1" in positions and "2" in positions + old1, new1 = positions["1"] + old2, new2 = positions["2"] + # Pin 1 old: (0 + 0, 0 + 3.81) + assert abs(old1[0] - 0) < 1e-4 + assert abs(old1[1] - 3.81) < 1e-4 + # Pin 2 old: (0 + 0, 0 - 3.81) + assert abs(old2[0] - 0) < 1e-4 + assert abs(old2[1] - (-3.81)) < 1e-4 + # New positions shifted by (10, 20) + assert abs(new1[0] - 10) < 1e-4 + assert abs(new1[1] - 23.81) < 1e-4 + assert abs(new2[0] - 10) < 1e-4 + assert abs(new2[1] - 16.19) < 1e-4 + + def test_resistor_rotated_90(self): + """Device:R at (100, 100) rot=90 — pins should be at (100+3.81, 100) and (100-3.81, 100).""" + sch = _make_sch_data([_make_symbol("R1", 100, 100, rotation=90)]) + positions = WireDragger.compute_pin_positions(sch, "R1", 100, 100) + old1, _ = positions["1"] + old2, _ = positions["2"] + # rotate(0, 3.81, 90) = (0*cos90 - 3.81*sin90, 0*sin90 + 3.81*cos90) = (-3.81, 0) + # Wait — pin 1 is at local (0, 3.81), rotated 90° CCW: + # x' = 0*cos90 - 3.81*sin90 = -3.81, y' = 0*sin90 + 3.81*cos90 ≈ 0 + # world: (100 - 3.81, 100 + 0) = (96.19, 100) + assert abs(old1[0] - 96.19) < 1e-3 + assert abs(old1[1] - 100) < 1e-3 + + def test_returns_empty_for_missing_component(self): + sch = _make_sch_data() + result = WireDragger.compute_pin_positions(sch, "MISSING", 0, 0) + assert result == {} + + def test_delta_is_consistent(self): + """new_xy - old_xy should equal (new_x - old_x, new_y - old_y) for any rotation.""" + sch = _make_sch_data([_make_symbol("R1", 50, 50, rotation=45)]) + positions = WireDragger.compute_pin_positions(sch, "R1", 60, 70) + for pin_num, (old_xy, new_xy) in positions.items(): + dx = new_xy[0] - old_xy[0] + dy = new_xy[1] - old_xy[1] + assert abs(dx - 10) < 1e-4, f"Pin {pin_num}: dx={dx}" + assert abs(dy - 20) < 1e-4, f"Pin {pin_num}: dy={dy}" + + +# --------------------------------------------------------------------------- +# TestDragWires +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestDragWires: + def test_no_wires_returns_zero_counts(self): + sch = _make_sch_data() + result = WireDragger.drag_wires(sch, {(0.0, 0.0): (10.0, 10.0)}) + assert result["endpoints_moved"] == 0 + assert result["wires_removed"] == 0 + + def test_wire_start_endpoint_moved(self): + wire = _make_wire(0, 3.81, 0, 10) + sch = _make_sch_data([wire]) + result = WireDragger.drag_wires(sch, {(0.0, 3.81): (10.0, 23.81)}) + assert result["endpoints_moved"] == 1 + assert result["wires_removed"] == 0 + # Find the updated wire in sch_data + updated = next(i for i in sch if isinstance(i, list) and i and i[0] == Symbol("wire")) + pts = next(s for s in updated[1:] if isinstance(s, list) and s and s[0] == Symbol("pts")) + xy1 = pts[1] + assert abs(xy1[1] - 10.0) < EPS + assert abs(xy1[2] - 23.81) < EPS + + def test_wire_end_endpoint_moved(self): + wire = _make_wire(0, 10, 0, -3.81) + sch = _make_sch_data([wire]) + result = WireDragger.drag_wires(sch, {(0.0, -3.81): (10.0, 16.19)}) + assert result["endpoints_moved"] == 1 + updated = next(i for i in sch if isinstance(i, list) and i and i[0] == Symbol("wire")) + pts = next(s for s in updated[1:] if isinstance(s, list) and s and s[0] == Symbol("pts")) + xy2 = pts[2] + assert abs(xy2[1] - 10.0) < EPS + assert abs(xy2[2] - 16.19) < EPS + + def test_zero_length_wire_removed(self): + """When both endpoints of a wire are moved to the same point, wire is deleted.""" + wire = _make_wire(0, 3.81, 0, -3.81) + sch = _make_sch_data([wire]) + # Both pins land at same position (degenerate move) + result = WireDragger.drag_wires( + sch, + { + (0.0, 3.81): (5.0, 5.0), + (0.0, -3.81): (5.0, 5.0), + }, + ) + assert result["wires_removed"] == 1 + wires_remaining = [i for i in sch if isinstance(i, list) and i and i[0] == Symbol("wire")] + assert len(wires_remaining) == 0 + + def test_unrelated_wire_not_touched(self): + """A wire whose endpoints don't match any old pin is not changed.""" + wire = _make_wire(50, 50, 60, 50) + sch = _make_sch_data([wire]) + original_start = (50.0, 50.0) + result = WireDragger.drag_wires(sch, {(0.0, 3.81): (10.0, 23.81)}) + assert result["endpoints_moved"] == 0 + updated = next(i for i in sch if isinstance(i, list) and i and i[0] == Symbol("wire")) + pts = next(s for s in updated[1:] if isinstance(s, list) and s and s[0] == Symbol("pts")) + xy1 = pts[1] + assert abs(xy1[1] - 50.0) < EPS + assert abs(xy1[2] - 50.0) < EPS + + def test_both_endpoints_on_moved_component(self): + """Wire connecting two pins of same component — both endpoints shift together.""" + wire = _make_wire(0, 3.81, 0, -3.81) + sch = _make_sch_data([wire]) + result = WireDragger.drag_wires( + sch, + { + (0.0, 3.81): (10.0, 23.81), + (0.0, -3.81): (10.0, 16.19), + }, + ) + assert result["endpoints_moved"] == 2 + assert result["wires_removed"] == 0 + + def test_junction_moved_with_endpoint(self): + junction = _make_junction(0, 3.81) + sch = _make_sch_data([junction]) + WireDragger.drag_wires(sch, {(0.0, 3.81): (10.0, 23.81)}) + updated_j = next(i for i in sch if isinstance(i, list) and i and i[0] == Symbol("junction")) + at_sub = next( + s for s in updated_j[1:] if isinstance(s, list) and s and s[0] == Symbol("at") + ) + assert abs(at_sub[1] - 10.0) < EPS + assert abs(at_sub[2] - 23.81) < EPS + + def test_junction_at_unrelated_position_not_touched(self): + junction = _make_junction(99, 99) + sch = _make_sch_data([junction]) + WireDragger.drag_wires(sch, {(0.0, 3.81): (10.0, 23.81)}) + updated_j = next(i for i in sch if isinstance(i, list) and i and i[0] == Symbol("junction")) + at_sub = next( + s for s in updated_j[1:] if isinstance(s, list) and s and s[0] == Symbol("at") + ) + assert abs(at_sub[1] - 99.0) < EPS + assert abs(at_sub[2] - 99.0) < EPS + + +# --------------------------------------------------------------------------- +# TestUpdateSymbolPosition +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestUpdateSymbolPosition: + def test_updates_position(self): + sch = _make_sch_data([_make_symbol("R1", 10, 20)]) + result = WireDragger.update_symbol_position(sch, "R1", 30, 40) + assert result is True + found = WireDragger.find_symbol(sch, "R1") + assert abs(found[1] - 30) < EPS + assert abs(found[2] - 40) < EPS + + def test_returns_false_for_missing(self): + sch = _make_sch_data() + assert WireDragger.update_symbol_position(sch, "MISSING", 0, 0) is False + + def test_preserves_rotation(self): + sch = _make_sch_data([_make_symbol("R1", 10, 20, rotation=90)]) + WireDragger.update_symbol_position(sch, "R1", 30, 40) + found = WireDragger.find_symbol(sch, "R1") + assert abs(found[3] - 90) < EPS # rotation preserved + + def test_property_labels_follow_symbol_move(self): + """Property (at ...) positions must shift by the same delta as the symbol.""" + sym = _make_symbol("R1", 100, 80) + sch = _make_sch_data([sym]) + + # Record initial property positions + prop_k = _sym("property") + at_k = _sym("at") + initial_positions = {} + for sub in sym[1:]: + if isinstance(sub, list) and sub and sub[0] == prop_k: + name = sub[1] + for psub in sub[2:]: + if isinstance(psub, list) and psub and psub[0] == at_k: + initial_positions[name] = (psub[1], psub[2]) + break + assert len(initial_positions) >= 2 # Reference and Value at minimum + + # Move component from (100, 80) to (120, 100) — delta (20, 20) + result = WireDragger.update_symbol_position(sch, "R1", 120, 100) + assert result is True + + # Verify each property shifted by (20, 20) + for sub in sym[1:]: + if isinstance(sub, list) and sub and sub[0] == prop_k: + name = sub[1] + for psub in sub[2:]: + if isinstance(psub, list) and psub and psub[0] == at_k: + expected_x = initial_positions[name][0] + 20 + expected_y = initial_positions[name][1] + 20 + assert ( + abs(psub[1] - expected_x) < EPS + ), f"{name} x: expected {expected_x}, got {psub[1]}" + assert ( + abs(psub[2] - expected_y) < EPS + ), f"{name} y: expected {expected_y}, got {psub[2]}" + break + + +# --------------------------------------------------------------------------- +# Integration tests +# --------------------------------------------------------------------------- + + +@pytest.mark.integration +class TestMoveWithWirePreservation: + """Integration tests using a real .kicad_sch file.""" + + def _make_schematic(self, extra_sexp=""): + """Copy empty.kicad_sch to a temp file and optionally append content.""" + tmp = Path(tempfile.mkdtemp()) / "test.kicad_sch" + shutil.copy(TEMPLATE_PATH, tmp) + if extra_sexp: + content = tmp.read_text(encoding="utf-8") + idx = content.rfind(")") + content = content[:idx] + "\n" + extra_sexp + "\n)" + tmp.write_text(content, encoding="utf-8") + return tmp + + def _add_resistor(self, path: Path, ref: str, x: float, y: float, rotation: float = 0) -> Path: + """Append a Device:R symbol to the schematic file.""" + import uuid + + u = str(uuid.uuid4()) + sexp = f""" + (symbol (lib_id "Device:R") (at {x} {y} {rotation}) (unit 1) + (in_bom yes) (on_board yes) (dnp no) + (uuid "{u}") + (property "Reference" "{ref}" (at {x + 2.032} {y} 90) + (effects (font (size 1.27 1.27))) + ) + (property "Value" "10k" (at {x} {y} 90) + (effects (font (size 1.27 1.27))) + ) + (property "Footprint" "" (at {x - 1.778} {y} 90) + (effects (font (size 1.27 1.27)) hide) + ) + (property "Datasheet" "~" (at {x} {y} 0) + (effects (font (size 1.27 1.27)) hide) + ) + (pin "1" (uuid "{uuid.uuid4()}")) + (pin "2" (uuid "{uuid.uuid4()}")) + (instances (project "test" (path "/" (reference "{ref}") (unit 1)))) + )""" + content = path.read_text(encoding="utf-8") + idx = content.rfind(")") + path.write_text(content[:idx] + "\n" + sexp + "\n)", encoding="utf-8") + return path + + def _add_wire(self, path: Path, x1, y1, x2, y2) -> Path: + """Append a wire to the schematic file.""" + import uuid + + wire_sexp = f""" + (wire (pts (xy {x1} {y1}) (xy {x2} {y2})) + (stroke (width 0) (type default)) + (uuid "{uuid.uuid4()}") + )""" + content = path.read_text(encoding="utf-8") + idx = content.rfind(")") + path.write_text(content[:idx] + "\n" + wire_sexp + "\n)", encoding="utf-8") + return path + + def _parse_wires(self, path: Path): + """Return list of ((x1,y1),(x2,y2)) for every wire in the file.""" + content = path.read_text(encoding="utf-8") + data = sexpdata.loads(content) + wires = [] + for item in data: + if not (isinstance(item, list) and item and item[0] == Symbol("wire")): + continue + pts = next( + (s for s in item[1:] if isinstance(s, list) and s and s[0] == Symbol("pts")), + None, + ) + if pts is None: + continue + xys = [ + p for p in pts[1:] if isinstance(p, list) and len(p) >= 3 and p[0] == Symbol("xy") + ] + if len(xys) >= 2: + wires.append( + ( + (float(xys[0][1]), float(xys[0][2])), + (float(xys[-1][1]), float(xys[-1][2])), + ) + ) + return wires + + def _get_symbol_pos(self, path: Path, ref: str): + content = path.read_text(encoding="utf-8") + data = sexpdata.loads(content) + found = WireDragger.find_symbol(data, ref) + if found is None: + return None + return found[1], found[2] + + def test_symbol_position_updated(self): + sch = self._make_schematic() + self._add_resistor(sch, "R1", 100, 100) + # Call handler directly + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + result = iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 120, "y": 130}, + }, + ) + assert result["success"], result.get("message") + pos = self._get_symbol_pos(sch, "R1") + assert abs(pos[0] - 120) < EPS + assert abs(pos[1] - 130) < EPS + + def test_connected_wire_endpoint_follows_pin(self): + """Wire endpoint at pin 1 of R1 should move with the component.""" + sch = self._make_schematic() + # R1 at (100, 100) — pin 1 at (100, 103.81) + self._add_resistor(sch, "R1", 100, 100) + self._add_wire(sch, 100, 103.81, 100, 120) # wire from pin 1 upward + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + result = iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 110, "y": 100}, + }, + ) + assert result["success"], result.get("message") + assert result["wiresMoved"] >= 1 + + wires = self._parse_wires(sch) + assert len(wires) == 1 + # Pin 1 new world position: (110 + 0, 100 + 3.81) = (110, 103.81) + w = wires[0] + endpoints = {w[0], w[1]} + new_pin1 = (110.0, 103.81) + assert any( + abs(ep[0] - new_pin1[0]) < 0.01 and abs(ep[1] - new_pin1[1]) < 0.01 for ep in endpoints + ), f"Expected pin endpoint near {new_pin1}, got {endpoints}" + + def test_unrelated_wire_unchanged(self): + """A wire not connected to R1 must not be modified.""" + sch = self._make_schematic() + self._add_resistor(sch, "R1", 100, 100) + self._add_wire(sch, 50, 50, 60, 50) # unrelated wire + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 110, "y": 110}, + }, + ) + + wires = self._parse_wires(sch) + unrelated = [(s, e) for s, e in wires if abs(s[0] - 50) < 0.01 and abs(s[1] - 50) < 0.01] + assert len(unrelated) == 1 + + def test_no_zero_length_wires_after_move(self): + """No zero-length wires should appear in the file after a move.""" + sch = self._make_schematic() + self._add_resistor(sch, "R1", 100, 100) + # Wire from pin 1 to pin 2 of same component (intra-component wire) + self._add_wire(sch, 100, 103.81, 100, 96.19) + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 110, "y": 100}, + }, + ) + + wires = self._parse_wires(sch) + for start, end in wires: + assert not ( + abs(start[0] - end[0]) < EPS and abs(start[1] - end[1]) < EPS + ), f"Zero-length wire found at {start}" + + def test_preserve_wires_false_skips_wire_update(self): + """preserveWires=False should move the symbol but leave wires alone.""" + sch = self._make_schematic() + self._add_resistor(sch, "R1", 100, 100) + self._add_wire(sch, 100, 103.81, 100, 120) + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + result = iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 110, "y": 100}, + "preserveWires": False, + }, + ) + assert result["success"] + assert result["wiresMoved"] == 0 + + # Wire should still start at old pin position + wires = self._parse_wires(sch) + assert len(wires) == 1 + endpoints = {wires[0][0], wires[0][1]} + old_pin1 = (100.0, 103.81) + assert any( + abs(ep[0] - old_pin1[0]) < 0.01 and abs(ep[1] - old_pin1[1]) < 0.01 for ep in endpoints + ), f"Wire should still be at {old_pin1}, got {endpoints}" + + def test_missing_component_returns_error(self): + sch = self._make_schematic() + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + result = iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "NOTHERE", + "position": {"x": 0, "y": 0}, + }, + ) + assert not result["success"] + assert "not found" in result.get("message", "").lower() + + +# --------------------------------------------------------------------------- +# TestSynthesizeTouchingPinWires (unit) +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestSynthesizeTouchingPinWires: + """Unit tests for WireDragger.synthesize_touching_pin_wires.""" + + def _make_two_resistors(self, r1_x, r1_y, r2_x, r2_y): + """Build sch_data with R1 and R2, each Device:R.""" + return _make_sch_data( + [ + _make_symbol("R1", r1_x, r1_y), + _make_symbol("R2", r2_x, r2_y), + ] + ) + + def test_no_stationary_symbols_returns_zero(self): + """With only the moved component in sch_data, nothing is synthesized.""" + sch = _make_sch_data([_make_symbol("R1", 0, 0)]) + pin_positions = WireDragger.compute_pin_positions(sch, "R1", 10, 20) + count = WireDragger.synthesize_touching_pin_wires(sch, "R1", pin_positions) + assert count == 0 + + def test_touching_pin_gap_generates_wire(self): + """ + R1 at (0, 0) pin2 at (0, -3.81). + R2 at (0, -7.62) pin1 at (0, -3.81). ← pins touch + Moving R1 to (10, 0) causes pin2 to move to (10, -3.81). + A wire from (0, -3.81) to (10, -3.81) should be synthesized. + """ + # R2 pin1 is at (0, -7.62 + 3.81) = (0, -3.81) + sch = self._make_two_resistors(0, 0, 0, -7.62) + + # Verify the touching: R1 pin2 old = (0, -3.81), R2 pin1 = (0, -3.81) + pin_positions = WireDragger.compute_pin_positions(sch, "R1", 10, 0) + old2, new2 = pin_positions["2"] + assert abs(old2[0] - 0) < 1e-3 and abs(old2[1] - (-3.81)) < 1e-3 + assert abs(new2[0] - 10) < 1e-3 and abs(new2[1] - (-3.81)) < 1e-3 + + wire_count_before = sum( + 1 for item in sch if isinstance(item, list) and item and item[0] == _sym("wire") + ) + count = WireDragger.synthesize_touching_pin_wires(sch, "R1", pin_positions) + assert count == 1, f"Expected 1 synthesized wire, got {count}" + + wires = [ + item for item in sch if isinstance(item, list) and item and item[0] == _sym("wire") + ] + assert len(wires) == wire_count_before + 1 + + # The new wire should span (0, -3.81) → (10, -3.81) + new_wire = wires[-1] + pts = next(s for s in new_wire[1:] if isinstance(s, list) and s and s[0] == _sym("pts")) + xys = [p for p in pts[1:] if isinstance(p, list) and len(p) >= 3 and p[0] == _sym("xy")] + assert len(xys) == 2 + endpoints = { + (round(float(xys[0][1]), 3), round(float(xys[0][2]), 3)), + (round(float(xys[1][1]), 3), round(float(xys[1][2]), 3)), + } + assert (0.0, -3.81) in endpoints, f"Expected (0, -3.81) in wire endpoints, got {endpoints}" + assert ( + 10.0, + -3.81, + ) in endpoints, f"Expected (10, -3.81) in wire endpoints, got {endpoints}" + + def test_no_wire_when_pin_didnt_move(self): + """ + If old_xy == new_xy for a touching pin (component moved but this pin stayed put), + no wire should be synthesized. + """ + # R1 at (0, 0), R2 at (0, -7.62) — pin2 of R1 and pin1 of R2 touch at (0, -3.81) + sch = self._make_two_resistors(0, 0, 0, -7.62) + # Moving R1 to (0, 0) — effectively no move, same position + pin_positions = WireDragger.compute_pin_positions(sch, "R1", 0, 0) + count = WireDragger.synthesize_touching_pin_wires(sch, "R1", pin_positions) + assert count == 0 + + def test_no_wire_when_rejoins_other_stationary_pin(self): + """ + If the moved pin's new position coincides with another stationary pin, + no wire should be synthesized (they touch again). + """ + # R1 at (0, 0), R2 at (0, -7.62), R3 at (10, -7.62) + # R1 pin2 was touching R2 pin1 at (0, -3.81). + # Moving R1 to (10, 0): pin2 lands at (10, -3.81) which is R3 pin1. + sch = _make_sch_data( + [ + _make_symbol("R1", 0, 0), + _make_symbol("R2", 0, -7.62), + _make_symbol("R3", 10, -7.62), + ] + ) + pin_positions = WireDragger.compute_pin_positions(sch, "R1", 10, 0) + count = WireDragger.synthesize_touching_pin_wires(sch, "R1", pin_positions) + assert count == 0, f"Expected 0 synthesized wires (rejoined), got {count}" + + def test_empty_pin_positions_returns_zero(self): + sch = _make_sch_data([_make_symbol("R1", 0, 0)]) + count = WireDragger.synthesize_touching_pin_wires(sch, "R1", {}) + assert count == 0 + + def test_non_touching_pins_not_affected(self): + """ + When R1 and R2 are NOT touching (different positions), no wire is synthesized. + """ + # R1 at (0, 0), R2 at (100, 100) — far apart + sch = self._make_two_resistors(0, 0, 100, 100) + pin_positions = WireDragger.compute_pin_positions(sch, "R1", 10, 0) + count = WireDragger.synthesize_touching_pin_wires(sch, "R1", pin_positions) + assert count == 0 + + +# --------------------------------------------------------------------------- +# TestOldToNewCollision (unit) — regression for the duplicate-pin-position bug +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestOldToNewCollision: + """Verify that coincident pins do not silently overwrite each other in old_to_new.""" + + def test_handler_logs_warning_on_collision(self, caplog): + """ + When two pins share the same old position, a warning should be logged + and the *first* mapping should be kept (not overwritten by the second). + """ + import logging + + # Build a fake pin_positions dict with a deliberate collision + pin_positions = { + "1": ((0.0, 3.81), (10.0, 23.81)), + "2": ((0.0, 3.81), (10.0, 16.19)), # same old_xy as pin "1" + } + + old_to_new = {} + with caplog.at_level(logging.WARNING, logger="kicad_interface"): + for _pin, (old_xy, new_xy) in pin_positions.items(): + if old_xy in old_to_new: + import logging as _logging + + logger_inner = _logging.getLogger("kicad_interface") + logger_inner.warning( + f"move_schematic_component: pin {_pin!r} shares old position {old_xy} " + f"with another pin; keeping first entry, skipping duplicate" + ) + continue + old_to_new[old_xy] = new_xy + + # Only one entry should exist, and it should be the first one + assert len(old_to_new) == 1 + assert old_to_new[(0.0, 3.81)] == (10.0, 23.81) + # Warning should have been logged + assert any("skipping duplicate" in r.message for r in caplog.records) + + +# --------------------------------------------------------------------------- +# TestTouchingPinIntegration (integration) +# --------------------------------------------------------------------------- + + +@pytest.mark.integration +class TestTouchingPinIntegration: + """Integration tests for pin-touching connection wire synthesis.""" + + def _make_schematic(self, extra_sexp=""): + """Copy empty.kicad_sch to a temp file.""" + tmp = Path(tempfile.mkdtemp()) / "test.kicad_sch" + shutil.copy(TEMPLATE_PATH, tmp) + if extra_sexp: + content = tmp.read_text(encoding="utf-8") + idx = content.rfind(")") + content = content[:idx] + "\n" + extra_sexp + "\n)" + tmp.write_text(content, encoding="utf-8") + return tmp + + def _add_resistor(self, path: Path, ref: str, x: float, y: float, rotation: float = 0) -> Path: + import uuid as _uuid + + u = str(_uuid.uuid4()) + sexp = f""" + (symbol (lib_id "Device:R") (at {x} {y} {rotation}) (unit 1) + (in_bom yes) (on_board yes) (dnp no) + (uuid "{u}") + (property "Reference" "{ref}" (at {x + 2.032} {y} 90) + (effects (font (size 1.27 1.27))) + ) + (property "Value" "10k" (at {x} {y} 90) + (effects (font (size 1.27 1.27))) + ) + (property "Footprint" "" (at {x - 1.778} {y} 90) + (effects (font (size 1.27 1.27)) hide) + ) + (property "Datasheet" "~" (at {x} {y} 0) + (effects (font (size 1.27 1.27)) hide) + ) + (pin "1" (uuid "{_uuid.uuid4()}")) + (pin "2" (uuid "{_uuid.uuid4()}")) + (instances (project "test" (path "/" (reference "{ref}") (unit 1)))) + )""" + content = path.read_text(encoding="utf-8") + idx = content.rfind(")") + path.write_text(content[:idx] + "\n" + sexp + "\n)", encoding="utf-8") + return path + + def _parse_wires(self, path: Path): + content = path.read_text(encoding="utf-8") + data = sexpdata.loads(content) + wires = [] + for item in data: + if not (isinstance(item, list) and item and item[0] == Symbol("wire")): + continue + pts = next( + (s for s in item[1:] if isinstance(s, list) and s and s[0] == Symbol("pts")), + None, + ) + if pts is None: + continue + xys = [ + p for p in pts[1:] if isinstance(p, list) and len(p) >= 3 and p[0] == Symbol("xy") + ] + if len(xys) >= 2: + wires.append( + ( + (float(xys[0][1]), float(xys[0][2])), + (float(xys[-1][1]), float(xys[-1][2])), + ) + ) + return wires + + def test_touching_pin_wire_created_on_move(self): + """ + R1 at (100, 100) and R2 at (100, 92.38) share a touching pin: + R1 pin2 = (100, 96.19), R2 pin1 = (100, 96.19). + Moving R1 to (110, 100) should synthesize a wire from (100, 96.19) to (110, 96.19). + """ + sch = self._make_schematic() + # R1 pin2 world position = 100 + (-3.81) = 96.19 + # R2 pin1 world position = 92.38 + 3.81 = 96.19 + self._add_resistor(sch, "R1", 100, 100) + self._add_resistor(sch, "R2", 100, 92.38) + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + result = iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 110, "y": 100}, + }, + ) + assert result["success"], result.get("message") + assert ( + result.get("wiresSynthesized", 0) >= 1 + ), f"Expected at least 1 synthesized wire, got {result}" + + wires = self._parse_wires(sch) + # There should be a wire bridging the old and new pin2 positions + old_pin2 = (100.0, 96.19) + new_pin2 = (110.0, 96.19) + bridging = [ + (s, e) + for s, e in wires + if ( + ( + abs(s[0] - old_pin2[0]) < 0.05 + and abs(s[1] - old_pin2[1]) < 0.05 + and abs(e[0] - new_pin2[0]) < 0.05 + and abs(e[1] - new_pin2[1]) < 0.05 + ) + or ( + abs(e[0] - old_pin2[0]) < 0.05 + and abs(e[1] - old_pin2[1]) < 0.05 + and abs(s[0] - new_pin2[0]) < 0.05 + and abs(s[1] - new_pin2[1]) < 0.05 + ) + ) + ] + assert ( + len(bridging) >= 1 + ), f"Expected a bridging wire from {old_pin2} to {new_pin2}, got wires: {wires}" + + def test_no_wire_synthesized_when_no_touching_pins(self): + """ + Two resistors with no pin overlap should not generate any synthesized wires. + """ + sch = self._make_schematic() + self._add_resistor(sch, "R1", 100, 100) + self._add_resistor(sch, "R2", 150, 150) # far away + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + result = iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 110, "y": 100}, + }, + ) + assert result["success"], result.get("message") + assert result.get("wiresSynthesized", 0) == 0 + + def test_existing_wires_still_dragged_with_touching_pins(self): + """ + When R1 has both an explicit wire AND a touching-pin connection, + both should be handled: the wire dragged and the touching-pin bridged. + """ + sch = self._make_schematic() + # R1 at (100, 100), R2 at (100, 92.38) — pin2 of R1 touches pin1 of R2 + self._add_resistor(sch, "R1", 100, 100) + self._add_resistor(sch, "R2", 100, 92.38) + + # Also add an explicit wire at pin1 of R1 (100, 103.81) going up + import uuid as _uuid + + wire_sexp = f""" + (wire (pts (xy 100 103.81) (xy 100 115)) + (stroke (width 0) (type default)) + (uuid "{_uuid.uuid4()}") + )""" + content = sch.read_text(encoding="utf-8") + idx = content.rfind(")") + sch.write_text(content[:idx] + "\n" + wire_sexp + "\n)", encoding="utf-8") + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + result = iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 110, "y": 100}, + }, + ) + assert result["success"], result.get("message") + assert result.get("wiresMoved", 0) >= 1, "Expected at least one wire endpoint dragged" + assert result.get("wiresSynthesized", 0) >= 1, "Expected at least one touching-pin wire" diff --git a/src/tools/schematic.ts b/src/tools/schematic.ts index ad75abd..eec672a 100644 --- a/src/tools/schematic.ts +++ b/src/tools/schematic.ts @@ -717,32 +717,40 @@ Note: operates on .kicad_sch files only. To modify a PCB footprint use edit_comp }, ); - // Move schematic component + // Move a placed symbol, dragging connected wires server.tool( "move_schematic_component", - "Move a placed symbol to a new position in the schematic.", + "Move a placed symbol to a new position in the schematic. By default (preserveWires=true) wire endpoints touching the component's pins are stretched to follow the new position.", { schematicPath: z.string().describe("Path to the .kicad_sch file"), reference: z.string().describe("Reference designator (e.g., R1, U1)"), position: z - .object({ - x: z.number(), - y: z.number(), - }) - .describe("New position"), + .object({ x: z.number(), y: z.number() }) + .describe("New position in schematic mm coordinates"), + preserveWires: z + .boolean() + .optional() + .describe("Stretch connected wire endpoints to follow the move (default true)"), }, async (args: { schematicPath: string; reference: string; position: { x: number; y: number }; + preserveWires?: boolean; }) => { const result = await callKicadScript("move_schematic_component", args); if (result.success) { + const moved = result.wiresMoved ?? 0; + const removed = result.wiresRemoved ?? 0; return { content: [ { type: "text", - text: `Moved ${args.reference} from (${result.oldPosition.x}, ${result.oldPosition.y}) to (${result.newPosition.x}, ${result.newPosition.y})`, + text: + `Moved ${args.reference} from (${result.oldPosition.x}, ${result.oldPosition.y}) ` + + `to (${result.newPosition.x}, ${result.newPosition.y})` + + (moved > 0 ? `, ${moved} wire endpoint(s) updated` : "") + + (removed > 0 ? `, ${removed} zero-length wire(s) removed` : ""), }, ], }; diff --git a/tests/test_ts_tool_registry.py b/tests/test_ts_tool_registry.py new file mode 100644 index 0000000..bcf5b1f --- /dev/null +++ b/tests/test_ts_tool_registry.py @@ -0,0 +1,61 @@ +""" +Regression test: no MCP tool name is registered more than once across all +TypeScript tool files in src/tools/. + +This caught a real bug where move_schematic_component was registered twice +(once in the original code and once in the PR adding wire-preservation), +causing the server to fail on startup with: + Error: Tool move_schematic_component is already registered +""" + +import re +from collections import Counter +from pathlib import Path + +import pytest + +SRC_TOOLS_DIR = Path(__file__).parent.parent / "src" / "tools" + +# Pattern matches the tool-name argument to server.tool( +# server.tool( +# "some_tool_name", +_SERVER_TOOL_RE = re.compile(r'server\.tool\(\s*["\']([a-zA-Z0-9_]+)["\']') + + +@pytest.mark.unit +class TestTsToolRegistry: + def _collect_registrations(self): + """Return list of (tool_name, file, line_no) for every server.tool() call.""" + registrations = [] + for ts_file in sorted(SRC_TOOLS_DIR.glob("**/*.ts")): + text = ts_file.read_text(encoding="utf-8") + for m in _SERVER_TOOL_RE.finditer(text): + line_no = text[: m.start()].count("\n") + 1 + registrations.append((m.group(1), ts_file.name, line_no)) + return registrations + + def test_no_duplicate_tool_names(self): + """Every tool name must appear exactly once across all TS tool files.""" + registrations = self._collect_registrations() + assert registrations, "No server.tool() calls found — check SRC_TOOLS_DIR path" + + counts = Counter(name for name, _, _ in registrations) + duplicates = {name: count for name, count in counts.items() if count > 1} + + if duplicates: + details = [] + for dup_name in sorted(duplicates): + locations = [ + f" {fname}:{line}" for name, fname, line in registrations if name == dup_name + ] + details.append(f"{dup_name} ({duplicates[dup_name]}x):\n" + "\n".join(locations)) + pytest.fail( + "Duplicate MCP tool registrations found — server will fail to start:\n\n" + + "\n\n".join(details) + ) + + def test_tool_files_exist(self): + """Sanity check: src/tools/ directory must be present and contain TS files.""" + assert SRC_TOOLS_DIR.is_dir(), f"src/tools/ not found at {SRC_TOOLS_DIR}" + ts_files = list(SRC_TOOLS_DIR.glob("**/*.ts")) + assert ts_files, "No .ts files found in src/tools/"