Merge pull request #139 from mixelpixx/fix/post-pr88-regressions

Fix two regressions from PR #88 (rotate/mirror)

python/commands/pin_locator.py

@@ -280,12 +280,12 @@ class PinLocator:
             pin_def_angle = pins[pin_number].get("angle", 0)
 
             # Mirror flips the angle before applying symbol rotation.
-            # mirror_x negates the Y component → reflects angle across X axis → negate angle.
+            # mirror_x flips the X component of local vectors → reflects across Y axis → 180 - angle.
-            # mirror_y negates the X component → reflects angle across Y axis → 180 - angle.
+            # mirror_y flips the Y component of local vectors → reflects across X axis → negate angle.
             if mirror_x:
-                pin_def_angle = (-pin_def_angle) % 360
-            if mirror_y:
                 pin_def_angle = (180 - pin_def_angle) % 360
+            if mirror_y:
+                pin_def_angle = (-pin_def_angle) % 360
 
             absolute_angle = (pin_def_angle + symbol_rotation) % 360
             return absolute_angle
@@ -374,9 +374,13 @@ class PinLocator:
             from commands.wire_dragger import WireDragger
 
             abs_x, abs_y = WireDragger.pin_world_xy(
-                pin_data["x"], pin_data["y"],
+                pin_data["x"],
-                symbol_x, symbol_y,
+                pin_data["y"],
-                symbol_rotation, mirror_x, mirror_y,
+                symbol_x,
+                symbol_y,
+                symbol_rotation,
+                mirror_x,
+                mirror_y,
             )
 
             logger.info(f"Pin {symbol_reference}/{pin_number} located at ({abs_x}, {abs_y})")

python/commands/wire_manager.py

@@ -306,25 +306,41 @@ class WireManager:
             True if successful, False otherwise
         """
         try:
-            from skip import Schematic
+            with open(schematic_path, "r", encoding="utf-8") as f:
-            from sexpdata import Symbol as SexpSymbol
+                sch_content = f.read()
 
-            schematic = Schematic(str(schematic_path))
+            sch_data = sexpdata.loads(sch_content)
 
-            existing_labels = list(schematic.label)
+            # Orientation-aware justify: KiCAD flips horizontal alignment for 180°/270°
-            if not existing_labels:
+            justify_h = Symbol("right") if orientation in (180, 270) else Symbol("left")
-                logger.warning("No existing labels to clone from; falling back to sexpdata")
-                raise RuntimeError("no existing labels")
 
-            new_label = existing_labels[0].clone()
+            label_sexp = [
-            new_label.value = text
+                Symbol(label_type),
-            new_label.at.value = [position[0], position[1], orientation]
+                text,
+                [Symbol("at"), position[0], position[1], orientation],
+                [
+                    Symbol("effects"),
+                    [Symbol("font"), [Symbol("size"), 1.27, 1.27]],
+                    [Symbol("justify"), justify_h, Symbol("bottom")],
+                ],
+                [Symbol("uuid"), str(uuid.uuid4())],
+            ]
 
-            # justify: left for 0°/90°, right for 180°/270° (matches KiCAD convention)
+            sheet_instances_index = None
-            justify_val = "right" if orientation in (180, 270) else "left"
+            for i, item in enumerate(sch_data):
-            new_label.effects.justify._tree[1] = SexpSymbol(justify_val)
+                if isinstance(item, list) and len(item) > 0 and item[0] == _SYM_SHEET_INSTANCES:
+                    sheet_instances_index = i
+                    break
+
+            if sheet_instances_index is None:
+                # Sub-sheets in hierarchical designs don't have (sheet_instances).
+                sheet_instances_index = len(sch_data)
+
+            sch_data.insert(sheet_instances_index, label_sexp)
+
+            with open(schematic_path, "w", encoding="utf-8") as f:
+                f.write(sexpdata.dumps(sch_data))
 
-            schematic.write(str(schematic_path))
             logger.info(f"Successfully added label '{text}' to {schematic_path.name}")
             return True
 

tests/test_add_label_empty_schematic.py

@@ -0,0 +1,105 @@
+"""
+Regression tests for WireManager.add_label on schematics with zero existing labels.
+
+Bug: PR #88 rewrote add_label to clone an existing label via kicad-skip; on a
+fresh schematic with no labels this returns False. The fix restores a hand-built
+sexpdata path (without the spurious fields_autoplaced token) as the primary path.
+"""
+
+import importlib.util
+import os
+import sys
+from unittest.mock import MagicMock
+
+import sexpdata
+from sexpdata import Symbol
+
+# Stub heavy / optional deps before loading wire_manager
+for modname in ("pcbnew", "skip"):
+    sys.modules.setdefault(modname, MagicMock())
+
+_wm_spec = importlib.util.spec_from_file_location(
+    "wire_manager",
+    os.path.join(os.path.dirname(__file__), "..", "python", "commands", "wire_manager.py"),
+)
+_wm_mod = importlib.util.module_from_spec(_wm_spec)
+_wm_spec.loader.exec_module(_wm_mod)
+WireManager = _wm_mod.WireManager
+
+
+_EMPTY_SCH = """\
+(kicad_sch (version 20250114) (generator "test")
+  (lib_symbols)
+  (sheet_instances
+    (path "/" (page "1"))
+  )
+)
+"""
+
+
+def _find_label(sch_data, text):
+    for item in sch_data:
+        if (
+            isinstance(item, list)
+            and len(item) >= 2
+            and isinstance(item[0], Symbol)
+            and str(item[0]) == "label"
+            and item[1] == text
+        ):
+            return item
+    return None
+
+
+def _has_token(label_sexp, name):
+    for part in label_sexp[2:]:
+        if isinstance(part, list) and part and isinstance(part[0], Symbol) and str(part[0]) == name:
+            return True
+    return False
+
+
+def _find_subtoken(label_sexp, name):
+    for part in label_sexp[2:]:
+        if isinstance(part, list) and part and isinstance(part[0], Symbol) and str(part[0]) == name:
+            return part
+    return None
+
+
+def test_add_label_to_empty_schematic_succeeds(tmp_path):
+    sch_path = tmp_path / "empty.kicad_sch"
+    sch_path.write_text(_EMPTY_SCH)
+
+    ok = WireManager.add_label(sch_path, "TEST", [50.0, 50.0], "label", 0)
+    assert ok is True
+
+    sch_data = sexpdata.loads(sch_path.read_text())
+    label = _find_label(sch_data, "TEST")
+    assert label is not None, 'Expected a (label "TEST" ...) token in the file'
+
+    # The fields_autoplaced token must NOT be present (regression guard for PR #88's bug).
+    assert not _has_token(
+        label, "fields_autoplaced"
+    ), "fields_autoplaced should not be emitted by add_label"
+
+
+def test_add_label_orientation_180_uses_right_bottom_justify(tmp_path):
+    sch_path = tmp_path / "empty180.kicad_sch"
+    sch_path.write_text(_EMPTY_SCH)
+
+    ok = WireManager.add_label(sch_path, "NETR", [60.0, 60.0], "label", 180)
+    assert ok is True
+
+    sch_data = sexpdata.loads(sch_path.read_text())
+    label = _find_label(sch_data, "NETR")
+    assert label is not None
+
+    effects = _find_subtoken(label, "effects")
+    assert effects is not None, "label must carry an (effects ...) token"
+
+    justify = _find_subtoken(effects, "justify")
+    assert justify is not None, "effects must carry a (justify ...) token"
+
+    justify_vals = [str(t) for t in justify[1:] if isinstance(t, Symbol)]
+    assert justify_vals == [
+        "right",
+        "bottom",
+    ], f"orientation=180 should produce 'right bottom' justify, got {justify_vals}"

tests/test_get_pin_angle.py

@@ -0,0 +1,163 @@
+"""
+Matrix tests for PinLocator.get_pin_angle on a Device:R symbol.
+
+For each combination of (symbol_rotation, mirror, pin), we construct a real
+.kicad_sch fixture, then compare:
+  - actual:   PinLocator().get_pin_angle(...)
+  - expected: derived geometrically from WireDragger.pin_world_xy by extending
+              the pin one length unit along its library angle and measuring the
+              world-frame displacement direction.
+
+This characterizes the post-PR-#88 angle reflection logic. Cases that disagree
+with the geometric expectation are marked xfail with a "PR #88 regression
+candidate" annotation.
+"""
+
+import importlib.util
+import math
+import os
+import sys
+import textwrap
+from pathlib import Path
+from unittest.mock import MagicMock
+
+import pytest
+import sexpdata
+
+# ---------------------------------------------------------------------------
+# Module loading — bypass pcbnew, mirror the test_rotate_schematic_mirror style
+# ---------------------------------------------------------------------------
+_PYTHON_DIR = os.path.join(os.path.dirname(__file__), "..", "python")
+if _PYTHON_DIR not in sys.path:
+    sys.path.insert(0, _PYTHON_DIR)
+
+# Stub pcbnew before importing pin_locator (skip is real and works fine)
+sys.modules.setdefault("pcbnew", MagicMock())
+
+_pl_spec = importlib.util.spec_from_file_location(
+    "pin_locator_under_test",
+    os.path.join(_PYTHON_DIR, "commands", "pin_locator.py"),
+)
+_pl_mod = importlib.util.module_from_spec(_pl_spec)
+_pl_spec.loader.exec_module(_pl_mod)
+PinLocator = _pl_mod.PinLocator
+
+_wd_spec = importlib.util.spec_from_file_location(
+    "wire_dragger_under_test",
+    os.path.join(_PYTHON_DIR, "commands", "wire_dragger.py"),
+)
+_wd_mod = importlib.util.module_from_spec(_wd_spec)
+_wd_spec.loader.exec_module(_wd_mod)
+WireDragger = _wd_mod.WireDragger
+
+
+# ---------------------------------------------------------------------------
+# Device:R pin definitions (per python/templates/empty.kicad_sch)
+#   pin 1: (0, 3.81), library angle 270, length 1.27
+#   pin 2: (0, -3.81), library angle 90, length 1.27
+# ---------------------------------------------------------------------------
+PIN_DEFS = {
+    "1": {"x": 0.0, "y": 3.81, "angle": 270.0, "length": 1.27},
+    "2": {"x": 0.0, "y": -3.81, "angle": 90.0, "length": 1.27},
+}
+
+SYMBOL_X = 100.0
+SYMBOL_Y = 100.0
+
+
+def _make_sch_text(rotation: float, mirror: str | None) -> str:
+    mirror_line = ""
+    if mirror == "x":
+        mirror_line = "(mirror x)"
+    elif mirror == "y":
+        mirror_line = "(mirror y)"
+
+    return textwrap.dedent(f"""\
+        (kicad_sch (version 20250114) (generator "test")
+          (lib_symbols
+            (symbol "Device:R" (pin_numbers hide) (pin_names (offset 0))
+              (symbol "R_1_1"
+                (pin passive line (at 0 3.81 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 -3.81 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 {SYMBOL_X} {SYMBOL_Y} {rotation})
+            {mirror_line}
+            (property "Reference" "R1" (at {SYMBOL_X} {SYMBOL_Y} 0))
+            (property "Value" "10k" (at {SYMBOL_X} {SYMBOL_Y} 0))
+          )
+        )
+    """)
+
+
+def _write_sch(tmp_path: Path, rotation: float, mirror: str | None) -> Path:
+    p = tmp_path / f"r_rot{int(rotation)}_mirror{mirror or 'none'}.kicad_sch"
+    p.write_text(_make_sch_text(rotation, mirror))
+    return p
+
+
+def _expected_stub_angle(pin_num: str, rotation: float, mirror: str | None) -> float:
+    """Geometrically expected outward angle: extend in library coords by +length
+    along library angle, transform to world, take atan2 of displacement."""
+    pin = PIN_DEFS[pin_num]
+    px, py = pin["x"], pin["y"]
+    lib_angle_rad = math.radians(pin["angle"])
+    ox = px + pin["length"] * math.cos(lib_angle_rad)
+    oy = py + pin["length"] * math.sin(lib_angle_rad)
+
+    mirror_x = mirror == "x"
+    mirror_y = mirror == "y"
+
+    wx_pin, wy_pin = WireDragger.pin_world_xy(
+        px, py, SYMBOL_X, SYMBOL_Y, rotation, mirror_x, mirror_y
+    )
+    wx_out, wy_out = WireDragger.pin_world_xy(
+        ox, oy, SYMBOL_X, SYMBOL_Y, rotation, mirror_x, mirror_y
+    )
+
+    deg = math.degrees(math.atan2(wy_out - wy_pin, wx_out - wx_pin)) % 360.0
+    # Snap to 0/90/180/270 (axis-aligned pins; FP noise tolerance)
+    snapped = round(deg / 90.0) * 90.0 % 360.0
+    if abs(((deg - snapped) + 540) % 360 - 180) < 1e-6:
+        # within tolerance
+        return snapped
+    return snapped
+
+
+# ---------------------------------------------------------------------------
+# Parametrized matrix
+# ---------------------------------------------------------------------------
+ROTATIONS = [0, 90, 180, 270]
+MIRRORS = [None, "x", "y"]
+PINS = ["1", "2"]
+
+
+@pytest.mark.parametrize("rotation", ROTATIONS)
+@pytest.mark.parametrize("mirror", MIRRORS)
+@pytest.mark.parametrize("pin_num", PINS)
+def test_get_pin_angle_matches_geometric_expectation(tmp_path, rotation, mirror, pin_num):
+    sch_path = _write_sch(tmp_path, rotation, mirror)
+    expected = _expected_stub_angle(pin_num, rotation, mirror)
+
+    locator = PinLocator()
+    actual = locator.get_pin_angle(sch_path, "R1", pin_num)
+
+    assert (
+        actual is not None
+    ), f"get_pin_angle returned None for rot={rotation} mirror={mirror} pin={pin_num}"
+
+    # Normalize both to [0, 360)
+    actual_n = actual % 360.0
+    expected_n = expected % 360.0
+
+    assert abs(((actual_n - expected_n) + 540) % 360 - 180) < 1e-3, (
+        f"actual={actual_n}, expected={expected_n} "
+        f"(rotation={rotation}, mirror={mirror}, pin={pin_num})"
+    )