fix(pcbnew): collab syncs footprint silkscreen text; un-no-op test-move helper
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) <noreply@anthropic.com>
This commit is contained in:
parent
2dc55c6138
commit
f60abaa349
2 changed files with 135 additions and 6 deletions
|
|
@ -21,6 +21,11 @@ import { test, expect } from "./fixtures";
|
||||||
|
|
||||||
const SEG1 = "44444444-0000-0000-0000-000000000001";
|
const SEG1 = "44444444-0000-0000-0000-000000000001";
|
||||||
const SEG2 = "44444444-0000-0000-0000-000000000002";
|
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
|
const SAMPLE_PCB = `(kicad_pcb
|
||||||
\t(version 20241229)
|
\t(version 20241229)
|
||||||
\t(generator "pcbnew")
|
\t(generator "pcbnew")
|
||||||
|
|
@ -32,10 +37,35 @@ const SAMPLE_PCB = `(kicad_pcb
|
||||||
\t(layers
|
\t(layers
|
||||||
\t\t(0 "F.Cu" signal)
|
\t\t(0 "F.Cu" signal)
|
||||||
\t\t(2 "B.Cu" signal)
|
\t\t(2 "B.Cu" signal)
|
||||||
|
\t\t(37 "F.SilkS" user)
|
||||||
\t\t(25 "Edge.Cuts" user)
|
\t\t(25 "Edge.Cuts" user)
|
||||||
\t)
|
\t)
|
||||||
\t(setup)
|
\t(setup)
|
||||||
\t(net 0 "")
|
\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 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}"))
|
\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.has(SEG1)).toBe(true);
|
||||||
expect(byId.get(SEG1)!.type).toBe("PCB_TRACK");
|
expect(byId.get(SEG1)!.type).toBe("PCB_TRACK");
|
||||||
expect(byId.get(SEG1)!.x).toBe(50_800_000); // 50.8mm × 1e6 IU (nm)
|
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);
|
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(echoes, "apply() must not echo a local onDelta").toHaveLength(0);
|
||||||
expect(hasAbort(testLogger), "no WASM abort").toBe(false);
|
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)", () => {
|
test.describe("pcbnew collab bridge — two tabs (BroadcastChannel)", () => {
|
||||||
|
|
|
||||||
|
|
@ -18,8 +18,11 @@
|
||||||
#include <footprint.h>
|
#include <footprint.h>
|
||||||
#include <pad.h>
|
#include <pad.h>
|
||||||
#include <pcb_track.h>
|
#include <pcb_track.h>
|
||||||
|
#include <pcb_field.h>
|
||||||
|
#include <pcb_text.h>
|
||||||
#include <pcb_group.h>
|
#include <pcb_group.h>
|
||||||
#include <zone.h>
|
#include <zone.h>
|
||||||
|
#include <eda_text.h>
|
||||||
#include <pcb_edit_frame.h>
|
#include <pcb_edit_frame.h>
|
||||||
#include <kiway_player.h>
|
#include <kiway_player.h>
|
||||||
#include <kiway.h>
|
#include <kiway.h>
|
||||||
|
|
@ -99,14 +102,36 @@ bool isTrackType( KICAD_T t )
|
||||||
return t == PCB_TRACE_T || t == PCB_ARC_T || t == PCB_VIA_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
|
// Iterate every board item the bridge syncs: the top-level items (tracks, footprints, drawings,
|
||||||
// child items (pads, fp text) are intentionally NOT visited individually — they move with their
|
// zones, groups) PLUS each footprint's TEXT children (fields = reference/value/user, and graphic
|
||||||
// parent footprint, so the bridge syncs the footprint as a unit (its uuid in m_itemByIdCache).
|
// 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 <typename Fn>
|
template <typename Fn>
|
||||||
void forEachTopItem( BOARD& aBoard, Fn&& aFn )
|
void forEachTopItem( BOARD& aBoard, Fn&& aFn )
|
||||||
{
|
{
|
||||||
for( PCB_TRACK* t : aBoard.Tracks() ) aFn( static_cast<BOARD_ITEM*>( t ) );
|
for( PCB_TRACK* t : aBoard.Tracks() ) aFn( static_cast<BOARD_ITEM*>( t ) );
|
||||||
for( FOOTPRINT* f : aBoard.Footprints() ) aFn( static_cast<BOARD_ITEM*>( f ) );
|
|
||||||
|
for( FOOTPRINT* f : aBoard.Footprints() )
|
||||||
|
{
|
||||||
|
aFn( static_cast<BOARD_ITEM*>( f ) );
|
||||||
|
|
||||||
|
for( PCB_FIELD* fld : f->GetFields() )
|
||||||
|
{
|
||||||
|
if( fld )
|
||||||
|
aFn( static_cast<BOARD_ITEM*>( fld ) );
|
||||||
|
}
|
||||||
|
|
||||||
|
for( BOARD_ITEM* g : f->GraphicalItems() )
|
||||||
|
{
|
||||||
|
if( g->Type() == PCB_TEXT_T )
|
||||||
|
aFn( g );
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
for( BOARD_ITEM* d : aBoard.Drawings() ) aFn( d );
|
for( BOARD_ITEM* d : aBoard.Drawings() ) aFn( d );
|
||||||
for( ZONE* z : aBoard.Zones() ) aFn( static_cast<BOARD_ITEM*>( z ) );
|
for( ZONE* z : aBoard.Zones() ) aFn( static_cast<BOARD_ITEM*>( z ) );
|
||||||
for( PCB_GROUP* g : aBoard.Groups() ) aFn( static_cast<BOARD_ITEM*>( g ) );
|
for( PCB_GROUP* g : aBoard.Groups() ) aFn( static_cast<BOARD_ITEM*>( g ) );
|
||||||
|
|
@ -138,6 +163,11 @@ json itemToJson( BOARD_ITEM* aItem )
|
||||||
j["width"] = tr->GetWidth();
|
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<EDA_TEXT*>( aItem ) )
|
||||||
|
j["text"] = toUtf8( txt->GetText() );
|
||||||
|
|
||||||
return j;
|
return j;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -350,6 +380,13 @@ void doApply( PCB_EDIT_FRAME* aFrame, const json& aDelta )
|
||||||
|
|
||||||
if( BOARD_ITEM* item = board->ResolveItem( id, /*allowNullptr*/ true ) )
|
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 );
|
commit.Remove( item );
|
||||||
staged = true;
|
staged = true;
|
||||||
}
|
}
|
||||||
|
|
@ -396,7 +433,11 @@ void doApply( PCB_EDIT_FRAME* aFrame, const json& aDelta )
|
||||||
s_applyingRemote = false;
|
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 )
|
void collabTestMove( PCB_EDIT_FRAME* aFrame, BOARD_ITEM* aItem, int aDx, int aDy )
|
||||||
{
|
{
|
||||||
BOARD_COMMIT commit( aFrame );
|
BOARD_COMMIT commit( aFrame );
|
||||||
|
|
@ -482,7 +523,17 @@ std::string kicadCollabTestMoveFirst( int aDx, int aDy )
|
||||||
return;
|
return;
|
||||||
|
|
||||||
movedId = toUtf8( item->m_Uuid.AsString() );
|
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<int, int> cor( [fr, item, aDx, aDy]( int ) -> int
|
||||||
|
{
|
||||||
|
collabTestMove( fr, item, aDx, aDy );
|
||||||
|
return 0;
|
||||||
|
} );
|
||||||
|
cor.Call( 0 );
|
||||||
|
} );
|
||||||
} );
|
} );
|
||||||
|
|
||||||
return movedId;
|
return movedId;
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue