diff --git a/kicad b/kicad index 92f18ef..a005a34 160000 --- a/kicad +++ b/kicad @@ -1 +1 @@ -Subproject commit 92f18ef4ed83f085dd63b3513372c07b0e428c86 +Subproject commit a005a34be0e0bc074c4d98bbaea51f59c0f7661f diff --git a/tests/kicad/roundtrip.spec.ts b/tests/kicad/roundtrip.spec.ts index 29e72f2..c7f1bcb 100644 --- a/tests/kicad/roundtrip.spec.ts +++ b/tests/kicad/roundtrip.spec.ts @@ -37,6 +37,14 @@ interface ToolCfg { empty: string; /** Known-volatile top-level tokens to ignore (serializer nondeterminism). */ ignoreTokens?: string[]; + /** + * uuids that live in the FILE but are legitimately absent from the items wire, + * because they identify the document rather than an item — e.g. a schematic's + * root `(uuid …)`, which travels in kdoc_meta/kdoc_layout, not kdoc_items. + * Listed explicitly per fixture so `expectWireMatchesFile` can subtract them + * without blanket-ignoring "the wire dropped an item", which is a real defect. + */ + wireOmitsUuids?: string[]; } type Mod = { @@ -161,6 +169,62 @@ async function roundTrip( return { orig, regen }; } +/** + * One boot: the file the tool would SAVE, and the per-item blobs it would put on + * the collab WIRE, taken from the same model at the same moment. + */ +async function fileAndWire( + context: BrowserContext, + cfg: ToolCfg, +): Promise<{ file: string; wire: string }> { + const page = await context.newPage(); + await bootOpen(page, cfg, cfg.fixture, "rt"); + const file = await saveRead(page, cfg, "orig_dump"); + const snap = await page.evaluate(() => window.Module.kicadCollabSnapshotItems()); + await page.close(); + + // Splice the blobs into one synthetic document so sexprDiff can index them by + // uuid. Non-footprint blobs already arrive wrapped in their own `(kicad_pcb …)` + // envelope; nesting is harmless — only uuid-bearing forms are compared, and the + // envelope's layers/version carry none. + const blobs: string[] = JSON.parse(snap).added.map((w: { sexpr: string }) => w.sexpr); + return { file, wire: `(${cfg.tool}_wire ${blobs.join("\n")})` }; +} + +/** + * THE invariant this suite was missing. `roundTrip` compares two FILE saves, so a + * wire blob written in a different dialect than the file writer round-trips + * perfectly and still corrupts the Y.Doc — which is exactly what happened: + * + * - pcbnew serialized wire footprints through CLIPBOARD_IO (CTL_FOR_CLIPBOARD), + * which unlike CTL_FOR_BOARD emits `(version …) (generator …) + * (generator_version …)` INSIDE every `(footprint …)`. Every footprint of + * every board drifted, permanently. + * - eeschema serialized symbols with `aForClipboard=true`, whose + * `MakeRelativeTo(currentSheet)` collapses `(instances … (path "/" …))` + * to `(path "")` — so materializing the Y.Doc would strip every symbol's sheet + * path, reference and unit. + * + * The Y.Doc is the source of truth for the FILE, so the two must agree token for + * token. NOTE: no `ignoreTokens` here on purpose — `generator_version` is exactly + * one of the tokens the pcbnew bug leaked, and ignoring it would re-mask it. (The + * fixture-vs-build version mismatch that `ignoreTokens` exists for lives on the + * ROOT form, which carries no uuid and is therefore never compared.) + */ +async function expectWireMatchesFile(context: BrowserContext, cfg: ToolCfg): Promise { + const { file, wire } = await fileAndWire(context, cfg); + const raw = sexprDiff(file, wire); + const omitted = new Set(cfg.wireOmitsUuids ?? []); + const diff = { + ...raw, + removed: raw.removed.filter((u) => !omitted.has(u)), + }; + expect( + diff.added.length === 0 && diff.removed.length === 0 && diff.changed.length === 0, + `wire blobs disagree with the file save:\n${JSON.stringify(diff, null, 2)}`, + ).toBe(true); +} + // ── Fixtures ──────────────────────────────────────────────────────────────── const PL: ToolCfg = { @@ -215,6 +279,68 @@ const SCH: ToolCfg = { ignoreTokens: ["generator_version"], }; +// A schematic carrying a real SYMBOL with `(instances …)`. The wire-vs-file test +// below needs one: the eeschema clipboard dialect rewrote the instance path +// relative to the current sheet, collapsing it to `(path "")`. A wire-only +// fixture (SCH above) cannot catch that — only symbols have instances. +const SCH_SYM: ToolCfg = { + ...SCH, + // The schematic's own root uuid is document identity (kdoc_meta/kdoc_layout), + // never an item on the items wire — so the file has it and the wire does not. + wireOmitsUuids: ["11111111-1111-1111-1111-111111111111"], + fixture: `(kicad_sch + (version 20250114) + (generator "eeschema") + (generator_version "9.0") + (uuid "11111111-1111-1111-1111-111111111111") + (paper "A4") + (lib_symbols + (symbol "Device:R" + (pin_numbers (hide yes)) + (pin_names (offset 0)) + (exclude_from_sim no) + (in_bom yes) + (on_board yes) + (property "Reference" "R" (at 2.032 0 90) (effects (font (size 1.27 1.27)))) + (property "Value" "R" (at 0 0 90) (effects (font (size 1.27 1.27)))) + (symbol "R_0_1" + (rectangle (start -1.016 -2.54) (end 1.016 2.54) + (stroke (width 0.254) (type default)) (fill (type none))) + ) + (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 100 100 0) + (unit 1) + (exclude_from_sim no) + (in_bom yes) + (on_board yes) + (dnp no) + (uuid "33333333-0000-0000-0000-000000000001") + (property "Reference" "R1" (at 102 99 0) (effects (font (size 1.27 1.27)) (justify left))) + (property "Value" "R" (at 102 101 0) (effects (font (size 1.27 1.27)) (justify left))) + (pin "1" (uuid "33333333-0000-0000-0000-0000000000a1")) + (pin "2" (uuid "33333333-0000-0000-0000-0000000000a2")) + (instances + (project "rt" + (path "/11111111-1111-1111-1111-111111111111" (reference "R1") (unit 1)) + ) + ) + ) + (sheet_instances (path "/" (page "1"))) +) +`, +}; + const PCB: ToolCfg = { tool: "pcbnew", // pcbnew-collab.html seeds kicad_common.json to skip the first-run setup wizard; @@ -357,6 +483,26 @@ test.describe("round trip: file → yjs → file", () => { expect(hasAbort(testLogger), "no WASM abort").toBe(false); }); + // The wire dialect must BE the file dialect — see expectWireMatchesFile. These + // two would both have failed before the CTL_FOR_BOARD / aForClipboard=false fix: + // pcbnew on the `(version)(generator)(generator_version)` triple inside every + // footprint, eeschema on `(instances … (path ""))`. + test("pcbnew wire blobs match the file save token for token", async ({ + context, + testLogger, + }) => { + await expectWireMatchesFile(context, PCB_FP); + expect(hasAbort(testLogger), "no WASM abort").toBe(false); + }); + + test("eeschema wire blobs keep the symbol's instance path", async ({ + context, + testLogger, + }) => { + await expectWireMatchesFile(context, SCH_SYM); + expect(hasAbort(testLogger), "no WASM abort").toBe(false); + }); + // REMAINING KNOWN GAP (ysync 0008 status, known limit 1 — tracked, not a test // bug): pcbnew track/via/zone/text APPLY rides the `(kicad_pcb …)` envelope // parse, the codebase's documented asyncify-fragile path (even a verbatim diff --git a/tests/web/fpedit-from-eeschema.spec.ts b/tests/web/fpedit-from-eeschema.spec.ts new file mode 100644 index 0000000..0e7e629 --- /dev/null +++ b/tests/web/fpedit-from-eeschema.spec.ts @@ -0,0 +1,138 @@ +import { test, expect, type Page } from '@playwright/test'; +import { clickByTooltip, stableShot, waitForWxApp } from '../e2e/utils/element-tracker'; + +/** + * Footprint Editor opened FROM a schematic session must show its libraries. + * + * Regression guard for the ysync-0010 §4 bug: `single_top.cpp` preloads libraries + * only for the face the app booted into. Booting `--frame=fpedit` (what + * `-/footprint_editor` does, and what every other fp-editor spec exercises) + * preloads FACE_PCB and works. But the eeschema toolbar button routes through + * `Kiway().Player( FRAME_FOOTPRINT_EDITOR )`, which starts FACE_PCB lazily at + * click time and never preloads — so nothing had walked the footprint libraries, + * `LIBRARY_MANAGER_ADAPTER::GetLibraryNames()` reported only rows whose status is + * LOADED (i.e. none), and `FP_TREE_SYNCHRONIZING_ADAPTER::Sync` skipped + * everything. The editor opened with a completely empty library tree. + * + * Fixed by `AsyncLoad()` + `BlockUntilLoaded()` in + * `FOOTPRINT_EDIT_FRAME::initLibraryTree()`. + * + * Two things this spec must NOT do, both learned the hard way: + * + * - Don't assert on `document.title`. It stays "… — Schematic Editor" even after + * a second editor frame opens (verified with Symbol Editor too), so a title + * poll reports failure on a working transition. The wx FRAME LIST is the + * signal: `ModEditFrame` appearing IS the Footprint Editor opening. + * - Don't use the `demo.kicad_sch` route. It boots its document from the Y.Doc + * and needs apps/sync (:3055), which this suite's stack does not run; the doc + * stalls at "untitled [Unsaved]" and FACE_PCB never starts. The fileless + * `-/eeschema` route needs no sync, and nothing here needs a loaded schematic — + * the bug is about which kiface preloaded its libraries. + * + * The tree assertion is a DELTA, not an absolute count: the schematic editor + * already renders its own dataviewitem rows (symbol libs), so "> 0" would pass + * even with a totally empty footprint tree. Growth past that baseline is exactly + * "the footprint tree got rows". + * + * Determinism: no blind waits — readiness comes from waitForWxApp, the app's own + * `kicadLibs` observable, and polling for the state each step produces. + */ + +const SCOPE = 'default'; +const BOOT_TIMEOUT = 180000; + +/** Names of the live wx top-level frames. */ +function frameNames(page: Page): Promise { + return page.evaluate(() => + (window as unknown as { wxElementRegistry: any }).wxElementRegistry + .findAll({}) + .filter((e: any) => /Frame$/.test(e.typeName || '')) + .map((e: any) => e.name as string), + ); +} + +/** Rendered library-tree rows across whatever frames are up. */ +function treeRowCount(page: Page): Promise { + return page.evaluate( + () => + (window as unknown as { wxElementRegistry: any }).wxElementRegistry.findAllRendered({ + elementType: 'dataviewitem', + }).length, + ); +} + +test.describe('footprint editor reached from eeschema', () => { + test.describe.configure({ timeout: 420000 }); + + test('opens with a populated library tree', async ({ page }) => { + const aborts: string[] = []; + page.on('console', (m) => { + if (/Aborted\(/i.test(m.text())) aborts.push(m.text()); + }); + + // 1) Boot the SCHEMATIC editor — the face whose preload does not cover footprints. + await page.goto(`/${SCOPE}/projects/demo/-/eeschema`); + await waitForWxApp(page, { timeout: BOOT_TIMEOUT }); + await expect + .poll(() => page.title(), { message: 'schematic editor up', timeout: BOOT_TIMEOUT }) + .toMatch(/Schematic Editor/i); + + // The libs bridge must exist before we cross over, or a miss below would be an + // app-plumbing failure rather than the kiface-preload bug under test. + await page.waitForFunction( + () => !!(window as unknown as { kicadLibs?: unknown }).kicadLibs, + null, + { timeout: 60000 }, + ); + + expect(await frameNames(page)).toEqual(['SchematicFrame']); + + // Baseline: the schematic editor's OWN tree rows (symbol libs) — see header. + // It must be SETTLED before we read it: those rows render asynchronously, and + // a baseline sampled too early (0) would weaken the delta below into "> 0", + // which the schematic's own rows could satisfy on their own. Two consecutive + // equal reads = settled. + let baseline = -1; + await expect + .poll( + async () => { + const n = await treeRowCount(page); + const settled = n === baseline; + baseline = n; + return settled; + }, + { message: 'schematic tree row count settled', timeout: 60000, intervals: [1000] }, + ) + .toBe(true); + + // 2) Cross to the Footprint Editor the way a user does: the toolbar button + // (ACTIONS::showFootprintEditor → Kiway().Player( FRAME_FOOTPRINT_EDITOR )). + expect( + await clickByTooltip(page, 'Create, delete and edit board footprints', { + elementType: 'tool', + }), + 'Footprint Editor toolbar button found and clicked', + ).toBe(true); + + // FACE_PCB starts lazily here — this is the first time the pcbnew kiface runs. + await expect + .poll(() => frameNames(page), { + message: 'Footprint Editor frame (ModEditFrame) opened', + timeout: BOOT_TIMEOUT, + }) + .toContain('ModEditFrame'); + + // 3) THE ASSERTION: its library tree is populated. Before the fix the frame + // opened exactly like this and the tree stayed empty, so the row count + // never moved off the schematic editor's baseline. + await expect + .poll(() => treeRowCount(page), { + message: `footprint library tree rows rendered (baseline was ${baseline})`, + timeout: 120000, + }) + .toBeGreaterThan(baseline); + + await stableShot(page, 'fpedit-from-eeschema-tree.png'); + expect(aborts, 'no WASM abort').toEqual([]); + }); +}); diff --git a/tests/web/symedit-from-eeschema.spec.ts b/tests/web/symedit-from-eeschema.spec.ts new file mode 100644 index 0000000..8f96367 --- /dev/null +++ b/tests/web/symedit-from-eeschema.spec.ts @@ -0,0 +1,112 @@ +import { test, expect, type Page } from '@playwright/test'; +import { clickByTooltip, waitForWxApp } from '../e2e/utils/element-tracker'; + +/** + * Symbol Editor opened from a schematic session must show its libraries. + * + * Companion to `fpedit-from-eeschema.spec.ts`, and deliberately the WEAKER of the + * two: this path is expected to be green already. It is here because + * `SYMBOL_EDIT_FRAME::SyncLibraries()` carries the same latent defect shape the + * footprint side had — + * + * SYMBOL_LIBRARY_ADAPTER* adapter = PROJECT_SCH::SymbolLibAdapter( &Prj() ); + * adapter->BlockUntilLoaded(); // ← no AsyncLoad() first + * + * — which `FOOTPRINT_LIST_IMPL::ReadFootprintFiles()` explicitly warns against + * ("AsyncLoad() must be called before BlockUntilLoaded() to ensure library + * loading is started"). `BlockUntilLoaded()` on a face that never preloaded + * returns immediately with nothing loaded, and the tree comes up empty. + * + * It cannot fail TODAY because every route to the Symbol Editor already has + * FACE_SCH running: `--frame=symedit` preloads it, and the only UI entry point is + * eeschema's own toolbar (`ACTIONS::showSymbolEditor` is appended solely by + * toolbars_sch_editor.cpp:201 — nothing in pcbnew/ references it). So this spec + * pins the currently-working behaviour; it turns into a real regression guard the + * moment a cross-kiface entry point is added. + * + * Same two traps as the footprint spec: assert on the wx FRAME LIST, not + * `document.title` (which stays "… — Schematic Editor" even after a second editor + * opens), and use the fileless `-/eeschema` route so no apps/sync is required. + */ + +const SCOPE = 'default'; +const BOOT_TIMEOUT = 180000; + +function frameNames(page: Page): Promise { + return page.evaluate(() => + (window as unknown as { wxElementRegistry: any }).wxElementRegistry + .findAll({}) + .filter((e: any) => /Frame$/.test(e.typeName || '')) + .map((e: any) => e.name as string), + ); +} + +function treeRowCount(page: Page): Promise { + return page.evaluate( + () => + (window as unknown as { wxElementRegistry: any }).wxElementRegistry.findAllRendered({ + elementType: 'dataviewitem', + }).length, + ); +} + +test.describe('symbol editor reached from eeschema', () => { + test.describe.configure({ timeout: 420000 }); + + test('opens with a populated library tree', async ({ page }) => { + const aborts: string[] = []; + page.on('console', (m) => { + if (/Aborted\(/i.test(m.text())) aborts.push(m.text()); + }); + + await page.goto(`/${SCOPE}/projects/demo/-/eeschema`); + await waitForWxApp(page, { timeout: BOOT_TIMEOUT }); + await expect + .poll(() => page.title(), { message: 'schematic editor up', timeout: BOOT_TIMEOUT }) + .toMatch(/Schematic Editor/i); + await page.waitForFunction( + () => !!(window as unknown as { kicadLibs?: unknown }).kicadLibs, + null, + { timeout: 60000 }, + ); + + expect(await frameNames(page)).toEqual(['SchematicFrame']); + + // Settle the baseline before reading it — see the footprint spec's header. + let baseline = -1; + await expect + .poll( + async () => { + const n = await treeRowCount(page); + const settled = n === baseline; + baseline = n; + return settled; + }, + { message: 'schematic tree row count settled', timeout: 60000, intervals: [1000] }, + ) + .toBe(true); + + expect( + await clickByTooltip(page, 'Create, delete and edit schematic symbols', { + elementType: 'tool', + }), + 'Symbol Editor toolbar button found and clicked', + ).toBe(true); + + await expect + .poll(() => frameNames(page), { + message: 'Symbol Editor frame (LibeditFrame) opened', + timeout: BOOT_TIMEOUT, + }) + .toContain('LibeditFrame'); + + await expect + .poll(() => treeRowCount(page), { + message: `symbol library tree rows rendered (baseline was ${baseline})`, + timeout: 120000, + }) + .toBeGreaterThan(baseline); + + expect(aborts, 'no WASM abort').toEqual([]); + }); +}); diff --git a/wasm/bindings/eeschema_embind.cpp b/wasm/bindings/eeschema_embind.cpp index 2007ee6..d2c7d38 100644 --- a/wasm/bindings/eeschema_embind.cpp +++ b/wasm/bindings/eeschema_embind.cpp @@ -355,10 +355,22 @@ void emitSheetChanged() }, s.c_str() ); } -// Serialize one live schematic item to its native s-expr via the clipboard -// formatter (the exact path Ctrl-C uses: a one-item SCH_SELECTION through -// SCH_IO_KICAD_SEXPR::Format). For a symbol the output also carries its -// (lib_symbols …) definition, just like a copy does. +// Serialize one live schematic item to its native s-expr via a one-item +// SCH_SELECTION through SCH_IO_KICAD_SEXPR::Format. For a symbol the output also +// carries its (lib_symbols …) definition (that prelude is emitted for any symbol +// in the selection, independent of aForClipboard). +// +// aForClipboard MUST be false. Clipboard mode is a LOSSY, paste-oriented dialect: +// it rewrites `(instances (project … (path …)))` relative to aRelativePath — so a +// symbol on the current sheet collapses to `(path "")` — takes the REFERENCE field +// from the per-sheet instance instead of the ordinal one, and keeps orphaned +// instance data (sch_io_kicad_sexpr.cpp saveSymbol: ~758-766, ~791-806, ~903). +// The Y.Doc is the source of truth for the FILE, so a wire blob must be byte-equal +// to that item's subtree in a full file save; clipboard form would silently strip +// every symbol's sheet path and unit/reference on materialize. +// +// aRelativePath is still required (Format wxCHECKs it non-null) but is unread on +// the aForClipboard=false path. std::string itemBlob( SCH_EDIT_FRAME* aFrame, SCH_ITEM* aItem ) { SCH_SELECTION sel; @@ -368,7 +380,7 @@ std::string itemBlob( SCH_EDIT_FRAME* aFrame, SCH_ITEM* aItem ) STRING_FORMATTER fmt; SCH_IO_KICAD_SEXPR plugin; plugin.Format( &sel, &aFrame->GetCurrentSheet(), aFrame->Schematic(), &fmt, - /*aForClipboard*/ true ); + /*aForClipboard*/ false ); return fmt.GetString(); } diff --git a/wasm/bindings/pcbnew_embind.cpp b/wasm/bindings/pcbnew_embind.cpp index 04d5595..65ddc0c 100644 --- a/wasm/bindings/pcbnew_embind.cpp +++ b/wasm/bindings/pcbnew_embind.cpp @@ -294,27 +294,55 @@ json itemToJson( BOARD_ITEM* aItem ) return j; } -// ── s-expr clipboard blob (the generic `added` mechanism) ──────────────────────────────────── +// ── s-expr item blob (the generic `added` mechanism) ───────────────────────────────────────── // // For added items beyond the natively-reconstructed PCB_TRACK (footprints, vias, zones, graphic -// shapes/text…), reuse KiCad's own copy/paste serializer, CLIPBOARD_IO. It Format()s a one-item -// selection exactly as Ctrl-C does — a bare `(footprint …)` for a footprint, or a fake -// `(kicad_pcb … )` envelope for everything else (the bare item tokens like -// `(segment`/`(via`/`(zone` are NOT accepted by the parser top-level, so the envelope is -// required). CLIPBOARD_IO normally talks to the system clipboard; SetWriter/SetReader redirect -// it to a string so it works headless / in wasm. +// shapes/text…), serialize one item with the BOARD writer's control set: a bare `(footprint …)` +// for a footprint, or a fake `(kicad_pcb … )` envelope for everything else (the +// bare item tokens like `(segment`/`(via`/`(zone` are NOT accepted by the parser top-level, so +// the envelope is required). +// +// The envelope still comes from CLIPBOARD_IO::SaveSelection, which is also what supplies the +// `(layers …)` block the parser needs; CLIPBOARD_IO normally talks to the system clipboard, so +// SetWriter/SetReader redirect it to a string to work headless / in wasm. Footprints take the +// dedicated path below instead — see blobForItem for why the clipboard dialect is wrong here. -// Serialize one live board item to a clipboard blob (used only for `added` payloads — NOT the +// A board writer we can point at a BOARD. PCB_IO_KICAD_SEXPR's default control set is +// CTL_FOR_BOARD — exactly what a .kicad_pcb save uses — but only CLIPBOARD_IO exposes a +// public SetBoard(); m_board is protected on PCB_IO, so a two-line subclass gets us the +// file writer without a fork change. +class WIRE_BOARD_IO : public PCB_IO_KICAD_SEXPR +{ +public: + explicit WIRE_BOARD_IO( BOARD* aBoard ) : PCB_IO_KICAD_SEXPR( CTL_FOR_BOARD ) + { + m_board = aBoard; + } +}; + +// Serialize one live board item to a wire blob (used only for `added` payloads — NOT the // diff unit, so `changed`/`removed` stay light and the blob never drives change detection). // -// Footprints DON'T go through SaveSelection: its "make safe to transfer" step copies the +// The blob MUST be byte-equal to that item's subtree in a full .kicad_pcb save: the Y.Doc is +// the source of truth for the FILE, so any writer difference is a permanent, unfixable drift +// and corrupts what a server-side materialize writes. +// +// That is why footprints use CTL_FOR_BOARD and not CLIPBOARD_IO's CTL_FOR_CLIPBOARD. The two +// differ by exactly CTL_OMIT_FOOTPRINT_VERSION (pcb_io_kicad_sexpr.h), so clipboard form emits +// (footprint "Lib:C1206" (version 20260206) (generator "pcbnew") (generator_version "10.0") …) +// — three tokens a board-embedded footprint never has (pcb_io_kicad_sexpr.cpp ~1201). Every +// footprint of every board drifted on those three lines. For non-footprints the two control +// sets are identical (the bit only gates footprint output), so the SaveSelection path below +// is already file-equivalent and keeps its `(kicad_pcb … )` envelope, which the +// parser requires — bare `(segment`/`(via`/`(zone` are not accepted at top level. +// +// Footprints also DON'T go through SaveSelection: its "make safe to transfer" step copies the // footprint, and FOOTPRINT's copy ctor ASSIGNS the mandatory fields into the new footprint's // freshly-constructed ones (`*existingField = *field`; EDA_ITEM::operator= keeps the target's // uuid) — so Reference/Value/Datasheet/Description would carry NEW uuids in every blob, // breaking the wire's identity-by-uuid (every emit would read as field remove+add, and round -// trips lose the field uuids). Instead we make the same safety copy ourselves, RESTORE the -// mandatory-field uuids from the source, and Format it directly — the same Format machinery -// SaveSelection uses internally, so asyncify behavior is identical. +// trips lose the field uuids). The copy ctor now restores those uuids itself (footprint.cpp), +// but we keep making the safety copy here so the live item is never mutated. std::string blobForItem( BOARD* aBoard, BOARD_ITEM* aItem ) { if( aItem->Type() == PCB_FOOTPRINT_T ) @@ -322,26 +350,17 @@ std::string blobForItem( BOARD* aBoard, BOARD_ITEM* aItem ) const FOOTPRINT* src = static_cast( aItem ); FOOTPRINT copy( *src ); - for( PCB_FIELD* field : copy.GetFields() ) - { - if( field->IsMandatory() ) - { - if( const PCB_FIELD* srcField = src->GetField( field->GetId() ) ) - const_cast( field->m_Uuid ) = srcField->m_Uuid; - } - } - // The rest of SaveSelection's footprint safety steps, minus the refPoint move // (the wire carries absolute positions) and minus SetNetCode(0): zeroing pad // nets is a paste-into-FOREIGN-board safety, but collab peers edit the SAME // board — nets must survive the wire. KiCad 10 formats pad nets by NAME and // the parser resolves by name against the receiver's board (creating the net // if missing), so no code remapping is needed on apply. - copy.SetLocked( false ); - - CLIPBOARD_IO io; + // + // NOTE: unlike SaveSelection we do NOT SetLocked( false ) — `(locked yes)` is + // real file content and dropping it would drift against the save. + WIRE_BOARD_IO io( aBoard ); STRING_FORMATTER fmt; - io.SetBoard( aBoard ); io.SetOutputFormatter( &fmt ); io.Format( © ); @@ -365,15 +384,58 @@ std::string blobForItem( BOARD* aBoard, BOARD_ITEM* aItem ) return out; } -// Reconstruct a board item from a clipboard blob. Parse() returns a bare FOOTPRINT*, or a BOARD* +// A bare `(footprint …)` blob carries no `(version …)`, but the parser NEEDS one: it starts at +// m_requiredVersion = 0, and several format decisions are gated on it — most visibly +// `if( m_requiredVersion < 20230620 ) field->SetVisible( false )` in the T_property case +// (pcb_io_kicad_sexpr_parser.cpp), which silently stamps `(hide yes)` onto every mandatory +// field of an applied footprint. +// +// The clipboard dialect got this for free because CTL_FOR_CLIPBOARD emits the version INSIDE +// the footprint form — but that token is not valid board-file content (see blobForItem), so we +// can't keep it in the Y.Doc. Instead re-supply it here, at parse time only: splice +// `(version N)` in right after `(footprint ""`, which is exactly where the clipboard +// writer put it. The Y.Doc body stays byte-identical to the file; only the wire→model decode +// sees the token. +static std::string withFootprintVersion( const std::string& aBlob ) +{ + static const std::string kHead = "(footprint"; + + if( aBlob.compare( 0, kHead.size(), kHead ) != 0 ) + return aBlob; // envelope blob — its (kicad_pcb …) carries a version + + if( aBlob.find( "(version " ) != std::string::npos ) + return aBlob; // already versioned (older peer, clipboard dialect) + + // Skip the quoted lib id that follows the head keyword, then inject. + size_t open = aBlob.find( '"', kHead.size() ); + + if( open == std::string::npos ) + return aBlob; + + size_t close = open + 1; + + while( close < aBlob.size() && aBlob[close] != '"' ) + close += ( aBlob[close] == '\\' ) ? 2 : 1; + + if( close >= aBlob.size() ) + return aBlob; + + return aBlob.substr( 0, close + 1 ) + + " (version " + std::to_string( SEXPR_BOARD_FILE_VERSION ) + ")" + + aBlob.substr( close + 1 ); +} + +// Reconstruct a board item from a wire blob. Parse() returns a bare FOOTPRINT*, or a BOARD* // (the `(kicad_pcb …)` envelope) holding the single item — in which case detach that item from // the throw-away board and hand back ownership. Returns nullptr on a parse failure (Parse catches // internally) or if no item is found. Runs inside the apply COROUTINE. -BOARD_ITEM* makeFromBlob( BOARD& aBoard, const std::string& aBlob ) +BOARD_ITEM* makeFromBlob( BOARD& aBoard, const std::string& aBlobIn ) { - if( aBlob.empty() ) + if( aBlobIn.empty() ) return nullptr; + const std::string aBlob = withFootprintVersion( aBlobIn ); + CLIPBOARD_IO io; io.SetBoard( &aBoard ); diff --git a/web/pcbjam-shared b/web/pcbjam-shared index db9529d..e044c86 160000 --- a/web/pcbjam-shared +++ b/web/pcbjam-shared @@ -1 +1 @@ -Subproject commit db9529d92f9b7a9014c52373c75462e7e5e51e3a +Subproject commit e044c86ea538a1ec6dd2120d1f85fcf9da6b8aa2 diff --git a/web/standalone/src/wasm/collab/drift-detect.ts b/web/standalone/src/wasm/collab/drift-detect.ts index 19482f9..014c88b 100644 --- a/web/standalone/src/wasm/collab/drift-detect.ts +++ b/web/standalone/src/wasm/collab/drift-detect.ts @@ -14,8 +14,9 @@ * looks like a user save (no upload, no peer-tab dirty flag). */ import { - docDelta, + compareSlots, type DriftReportBody, + driftDocDelta, fileToDoc, isEmptyKicadDelta, type Tool, @@ -58,10 +59,17 @@ const DEFAULT_EVERY_N = 50; /** Reports per session cap — a real reconciler bug must not flood the backend. */ const MAX_REPORTS_PER_SESSION = 20; -/** djb2 over the drift-defining JSON — dedupe key, not a security hash. */ +/** + * djb2 over the drift-defining JSON — dedupe key, not a security hash. + * + * `diff.reordered` and `layoutReordered` are excluded on purpose: they are not + * drift, and v2's order churn would otherwise change the key on every pass and + * defeat the "report a stable divergence once" rule. + */ function driftKey(body: DriftReportBody): string { + const { reordered: _reordered, ...diff } = body.diff; const canon = JSON.stringify({ - diff: body.diff, + diff, layoutChanged: body.layoutChanged, metaChanged: body.metaChanged, }); @@ -132,10 +140,13 @@ export function startDriftDetection(opts: DriftDetectOptions): DriftDetector { const wasmDoc = fileToDoc(text); const ydocDoc = yToDoc(opts.doc); - const diff = docDelta(ydocDoc, wasmDoc); - // docDelta covers items only; flag layout/preamble divergence separately. - const layoutChanged = - JSON.stringify(ydocDoc.layout) !== JSON.stringify(wasmDoc.layout); + // Order-only differences go to `diff.reordered` / `layoutReordered`: y-sexpr + // v2 reorders legitimately, so they are noise, not divergence (kicad-delta.ts). + const diff = driftDocDelta(ydocDoc, wasmDoc); + // driftDocDelta covers items only; flag layout/preamble divergence separately. + const layoutRelation = compareSlots(ydocDoc.layout, wasmDoc.layout); + const layoutChanged = layoutRelation === "different"; + const layoutReordered = layoutRelation === "reordered"; const metaChanged = ydocDoc.root !== wasmDoc.root; if (isEmptyKicadDelta(diff) && !layoutChanged && !metaChanged) return null; @@ -145,6 +156,7 @@ export function startDriftDetection(opts: DriftDetectOptions): DriftDetector { ydocDoc, diff, layoutChanged, + layoutReordered, metaChanged, // The docs above are version-blind (yToDoc normalizes v1/v2) — this is // the only signal of which storage encoding the Y.Doc actually used. @@ -166,7 +178,9 @@ export function startDriftDetection(opts: DriftDetectOptions): DriftDetector { if (body) { log( `[drift] ${opts.targetPath}: +${body.diff.added.length} ~${body.diff.updated.length} -${body.diff.removed.length}` + - `${body.layoutChanged ? " layout" : ""}${body.metaChanged ? " meta" : ""}`, + `${body.diff.reordered.length ? ` (${body.diff.reordered.length} reordered, not drift)` : ""}` + + `${body.layoutChanged ? " layout" : ""}${body.layoutReordered ? " layout-reordered" : ""}` + + `${body.metaChanged ? " meta" : ""}`, ); await reportDrift(opts.slug, body); }