From f60abaa3496924a5ace55f3cc347e41faa0d0f44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20T=C3=B6rcsv=C3=A1ri?= Date: Fri, 5 Jun 2026 17:13:47 +0200 Subject: [PATCH] fix(pcbnew): collab syncs footprint silkscreen text; un-no-op test-move helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug 1 — footprint child text now syncs. forEachTopItem also visits each footprint's TEXT children by their own uuid (GetFields() PCB_FIELDs + GraphicalItems() PCB_TEXT), so moving a silkscreen reference/value/user text — which leaves the footprint origin unchanged — produces a diff. Apply needed only a guard: the removed loop skips items with a parent footprint (a footprint delete cascades to its children, so don't double-remove). ResolveItem already resolves child uuids and BOARD_COMMIT::undoLevelItem rolls a child Modify up to the footprint, so Modify+SetPosition (in the existing COROUTINE) just works. itemToJson now carries text for EDA_TEXT items. Bug 2 — kicadCollabTestMoveFirst no longer no-ops. Its CallAfter now runs the move inside a COROUTINE fiber (like doApply) so the virtual BOARD_ITEM::Move dispatches instead of asyncify-no-opping. Tests: sample board gains a footprint with a Reference field + user fp_text; snapshot test asserts the footprint and both text children are emitted by uuid; a new test moves each child by uuid and asserts the footprint origin stays put. 3 pass, 1 skipped. Verified two-tab in the real app: C1's reference text move syncs A->B, no footprint moves. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/kicad/pcbnew-collab.spec.ts | 78 +++++++++++++++++++++++++++++++ wasm/bindings/pcbnew_embind.cpp | 63 ++++++++++++++++++++++--- 2 files changed, 135 insertions(+), 6 deletions(-) diff --git a/tests/kicad/pcbnew-collab.spec.ts b/tests/kicad/pcbnew-collab.spec.ts index 7d3cc59..49e2a70 100644 --- a/tests/kicad/pcbnew-collab.spec.ts +++ b/tests/kicad/pcbnew-collab.spec.ts @@ -21,6 +21,11 @@ import { test, expect } from "./fixtures"; const SEG1 = "44444444-0000-0000-0000-000000000001"; const SEG2 = "44444444-0000-0000-0000-000000000002"; +// A footprint and two of its TEXT children (a silkscreen Reference field + a user fp_text), each +// with a stable uuid so the test can move them independently of the footprint — the bug 1 case. +const FP1 = "66666666-0000-0000-0000-000000000001"; +const FP1_REF = "66666666-0000-0000-0000-0000000000aa"; // Reference field (PCB_FIELD, F.SilkS) +const FP1_TXT = "66666666-0000-0000-0000-0000000000cc"; // user fp_text (PCB_TEXT, F.SilkS) const SAMPLE_PCB = `(kicad_pcb \t(version 20241229) \t(generator "pcbnew") @@ -32,10 +37,35 @@ const SAMPLE_PCB = `(kicad_pcb \t(layers \t\t(0 "F.Cu" signal) \t\t(2 "B.Cu" signal) +\t\t(37 "F.SilkS" user) \t\t(25 "Edge.Cuts" user) \t) \t(setup) \t(net 0 "") +\t(footprint "TestLib:R" +\t\t(layer "F.Cu") +\t\t(uuid "${FP1}") +\t\t(at 100 100) +\t\t(attr smd) +\t\t(property "Reference" "R1" +\t\t\t(at 0 -4.2 0) +\t\t\t(layer "F.SilkS") +\t\t\t(uuid "${FP1_REF}") +\t\t\t(effects (font (size 1 1) (thickness 0.15))) +\t\t) +\t\t(property "Value" "R" +\t\t\t(at 0 4.6 0) +\t\t\t(layer "F.Fab") +\t\t\t(uuid "66666666-0000-0000-0000-0000000000bb") +\t\t\t(effects (font (size 1 1) (thickness 0.15))) +\t\t) +\t\t(fp_text user "HELLO" +\t\t\t(at 0 0 0) +\t\t\t(layer "F.SilkS") +\t\t\t(uuid "${FP1_TXT}") +\t\t\t(effects (font (size 1 1) (thickness 0.15))) +\t\t) +\t) \t(segment (start 50.8 50.8) (end 101.6 50.8) (width 0.2) (layer "F.Cu") (net 0) (uuid "${SEG1}")) \t(segment (start 50.8 76.2) (end 101.6 76.2) (width 0.2) (layer "F.Cu") (net 0) (uuid "${SEG2}")) ) @@ -117,6 +147,14 @@ test.describe("pcbnew collab bridge — single page", () => { expect(byId.has(SEG1)).toBe(true); expect(byId.get(SEG1)!.type).toBe("PCB_TRACK"); expect(byId.get(SEG1)!.x).toBe(50_800_000); // 50.8mm × 1e6 IU (nm) + // The footprint AND its text children are emitted individually by uuid (bug 1 fix) — that's + // what lets a silkscreen text move sync without the footprint moving. + expect(byId.has(FP1), "footprint present").toBe(true); + expect(byId.get(FP1)!.type).toBe("FOOTPRINT"); + expect(byId.has(FP1_REF), "footprint Reference field present").toBe(true); + expect(byId.get(FP1_REF)!.type).toBe("PCB_FIELD"); + expect(byId.has(FP1_TXT), "footprint user fp_text present").toBe(true); + expect(byId.get(FP1_TXT)!.type).toBe("PCB_TEXT"); expect(hasAbort(testLogger), "no WASM abort").toBe(false); }); @@ -207,6 +245,46 @@ test.describe("pcbnew collab bridge — single page", () => { expect(echoes, "apply() must not echo a local onDelta").toHaveLength(0); expect(hasAbort(testLogger), "no WASM abort").toBe(false); }); + + // Bug 1: a footprint's silkscreen text (a child item) can be moved independently of its + // footprint. apply resolves the child by uuid (BOARD::ResolveItem sees footprint children) and + // commits a Modify — which BOARD_COMMIT::undoLevelItem rolls up to the parent footprint — then + // SetPosition. The footprint itself does NOT move. Covers both the Reference field (PCB_FIELD) + // and a user fp_text (PCB_TEXT). + test("apply moves a footprint's silkscreen text child by uuid (footprint stays put)", async ({ + page, + testLogger, + }) => { + await bootAndOpen(page, "fptext"); + + const fpBefore = await page.evaluate((id) => window.Module.kicadCollabGetPos(id), FP1); + + for (const childId of [FP1_REF, FP1_TXT]) { + const before = await page.evaluate((id) => window.Module.kicadCollabGetPos(id), childId); + expect(before, `child ${childId} resolvable`).not.toBe(""); + const [cx, cy] = before.split(",").map(Number); + const ny = cy + 3_000_000; // +3mm in Y + + await page.evaluate( + ({ id, cx, ny }) => + window.Module.kicadCollabApply( + JSON.stringify({ changed: [{ id, x: cx, y: ny }], added: [], removed: [] }), + ), + { id: childId, cx, ny }, + ); + await expect + .poll(() => page.evaluate((id) => window.Module.kicadCollabGetPos(id), childId), { + timeout: 10000, + intervals: [200], + }) + .toBe(`${cx},${ny}`); + } + + // The footprint origin must be unchanged — only the child text moved. + const fpAfter = await page.evaluate((id) => window.Module.kicadCollabGetPos(id), FP1); + expect(fpAfter, "footprint did not move").toBe(fpBefore); + expect(hasAbort(testLogger), "no WASM abort").toBe(false); + }); }); test.describe("pcbnew collab bridge — two tabs (BroadcastChannel)", () => { diff --git a/wasm/bindings/pcbnew_embind.cpp b/wasm/bindings/pcbnew_embind.cpp index 3e4c770..6361247 100644 --- a/wasm/bindings/pcbnew_embind.cpp +++ b/wasm/bindings/pcbnew_embind.cpp @@ -18,8 +18,11 @@ #include #include #include +#include +#include #include #include +#include #include #include #include @@ -99,14 +102,36 @@ bool isTrackType( KICAD_T t ) return t == PCB_TRACE_T || t == PCB_ARC_T || t == PCB_VIA_T; } -// Iterate every TOP-LEVEL board item (tracks, footprints, drawings, zones, groups). Footprint -// child items (pads, fp text) are intentionally NOT visited individually — they move with their -// parent footprint, so the bridge syncs the footprint as a unit (its uuid in m_itemByIdCache). +// Iterate every board item the bridge syncs: the top-level items (tracks, footprints, drawings, +// zones, groups) PLUS each footprint's TEXT children (fields = reference/value/user, and graphic +// PCB_TEXT). The text children are visited by their OWN uuid because a silkscreen reference/value +// can be moved *independently* of its footprint (the footprint origin doesn't change, so syncing +// the footprint as a unit would miss it). Pads and footprint graphic shapes are NOT visited — they +// move only with the footprint. All these uuids live in BOARD::m_itemByIdCache, so apply resolves +// them directly. On a whole-footprint move the children also re-appear in the diff (their absolute +// positions changed); that's redundant but convergent, since apply uses absolute SetPosition. template void forEachTopItem( BOARD& aBoard, Fn&& aFn ) { for( PCB_TRACK* t : aBoard.Tracks() ) aFn( static_cast( t ) ); - for( FOOTPRINT* f : aBoard.Footprints() ) aFn( static_cast( f ) ); + + for( FOOTPRINT* f : aBoard.Footprints() ) + { + aFn( static_cast( f ) ); + + for( PCB_FIELD* fld : f->GetFields() ) + { + if( fld ) + aFn( static_cast( fld ) ); + } + + for( BOARD_ITEM* g : f->GraphicalItems() ) + { + if( g->Type() == PCB_TEXT_T ) + aFn( g ); + } + } + for( BOARD_ITEM* d : aBoard.Drawings() ) aFn( d ); for( ZONE* z : aBoard.Zones() ) aFn( static_cast( z ) ); for( PCB_GROUP* g : aBoard.Groups() ) aFn( static_cast( g ) ); @@ -138,6 +163,11 @@ json itemToJson( BOARD_ITEM* aItem ) j["width"] = tr->GetWidth(); } + // Text items (incl. footprint fields / graphic text): carry the string so a move diff is + // legible and a future text `added` can reconstruct. Position-only sync uses x/y above. + if( EDA_TEXT* txt = dynamic_cast( aItem ) ) + j["text"] = toUtf8( txt->GetText() ); + return j; } @@ -350,6 +380,13 @@ void doApply( PCB_EDIT_FRAME* aFrame, const json& aDelta ) if( BOARD_ITEM* item = board->ResolveItem( id, /*allowNullptr*/ true ) ) { + // A footprint text child appears in `removed` when its whole footprint was deleted + // (it vanished from the sender's snapshot). Removing the footprint cascades to its + // children, so don't also remove the child here — that would double-remove. (A rare + // child-only delete with the footprint kept is therefore not synced; acceptable.) + if( item->GetParentFootprint() ) + continue; + commit.Remove( item ); staged = true; } @@ -396,7 +433,11 @@ void doApply( PCB_EDIT_FRAME* aFrame, const json& aDelta ) s_applyingRemote = false; } -// Test/PoC move (the BOARD_COMMIT body for kicadCollabTestMoveFirst, deferred via CallAfter). +// Test/PoC move (the BOARD_COMMIT body for kicadCollabTestMoveFirst). Run inside a COROUTINE by +// the caller: `BOARD_ITEM::Move` is virtual, and dispatched off the app main stack (a bare +// CallAfter) it hits the asyncify call_indirect mis-dispatch and silently NO-OPS — the commit +// dirties the board but the item doesn't move. On the fiber stack (where native tool edits and +// doApply run) it dispatches correctly. (Same lesson as eeschema's devirtualized move.) void collabTestMove( PCB_EDIT_FRAME* aFrame, BOARD_ITEM* aItem, int aDx, int aDy ) { BOARD_COMMIT commit( aFrame ); @@ -482,7 +523,17 @@ std::string kicadCollabTestMoveFirst( int aDx, int aDy ) return; movedId = toUtf8( item->m_Uuid.AsString() ); - fr->CallAfter( [fr, item, aDx, aDy]() { collabTestMove( fr, item, aDx, aDy ); } ); + // Run on the app main stack (CallAfter) AND inside a COROUTINE fiber, so + // the virtual Move() dispatches instead of no-opping — same wrapping as + // kicadCollabApply's doApply. + fr->CallAfter( [fr, item, aDx, aDy]() { + COROUTINE cor( [fr, item, aDx, aDy]( int ) -> int + { + collabTestMove( fr, item, aDx, aDy ); + return 0; + } ); + cor.Call( 0 ); + } ); } ); return movedId;