diff --git a/docs/features/ysync-review/00-overview.md b/docs/features/ysync-review/00-overview.md index 76a3e52..2b9f84e 100644 --- a/docs/features/ysync-review/00-overview.md +++ b/docs/features/ysync-review/00-overview.md @@ -95,3 +95,4 @@ sending half emits NOTHING, not the bare removal the doc predicted). | 17 | [17-fixes-bugs-01-07.md](17-fixes-bugs-01-07.md) | Bugs 01–07 fixed & verified (2026-07-03); findings F5–F6; remaining follow-ups | | 18 | [18-miss08-opts-12-13.md](18-miss08-opts-12-13.md) | Miss 08 (lib_symbols + layout save-sync) + opts 12/13 implemented; opt 14 deferred (2026-07-03) | | 19 | [19-undo-option1-feasibility.md](19-undo-option1-feasibility.md) | Miss-09 option 1 (local-ops-only undo) feasibility research (2026-07-07): ~3–5 days, eeschema UUID-guard port is the core | +| 20 | [20-fix-miss09-collab-aware-undo.md](20-fix-miss09-collab-aware-undo.md) | Miss 09 FIXED (2026-07-07): SKIP_UNDO remote applies + eeschema UUID guard + binding-owned removed-item lifetime; 5/5 e2e | diff --git a/docs/features/ysync-review/09-miss-undo-not-collab-aware.md b/docs/features/ysync-review/09-miss-undo-not-collab-aware.md index bbe6d2a..ff355fa 100644 --- a/docs/features/ysync-review/09-miss-undo-not-collab-aware.md +++ b/docs/features/ysync-review/09-miss-undo-not-collab-aware.md @@ -1,7 +1,8 @@ # Design miss 09 — Undo is not collab-aware: Ctrl+Z reverts peers' work and re-broadcasts it **Severity:** design gap (converges, but with surprising and destructive UX) -**Status:** open decision +**Status:** FIXED 2026-07-07 — option 1 implemented, see +[20-fix-miss09-collab-aware-undo.md](20-fix-miss09-collab-aware-undo.md) ## Where diff --git a/docs/features/ysync-review/20-fix-miss09-collab-aware-undo.md b/docs/features/ysync-review/20-fix-miss09-collab-aware-undo.md new file mode 100644 index 0000000..f9f8199 --- /dev/null +++ b/docs/features/ysync-review/20-fix-miss09-collab-aware-undo.md @@ -0,0 +1,68 @@ +# Fix 20 — Miss 09 implemented: local-ops-only undo (option 1) + +**Status:** DONE, e2e-verified 2026-07-07 (5/5 firefox + collab/ysync regression suites) +**Design:** [09-miss-undo-not-collab-aware.md](09-miss-undo-not-collab-aware.md) option 1, +feasibility per [19-undo-option1-feasibility.md](19-undo-option1-feasibility.md) + +Remote applies no longer land on the receiving editor's undo stack: Ctrl+Z after a +peer's edit reverts your own last op, never the peer's, and the adopt "undo bomb" is +gone (adopt creates no undo entry at all — 09's option 3 history-barrier is moot). + +## What changed + +### KiCad fork + +- `eeschema/schematic_undo_redo.cpp` — **UUID stale-pointer guard** in + `PutDataInPreviousState`, mirroring pcbnew's: every picker (except `DELETED`, + `PAGESETTINGS`, `REPEAT_ITEM`, and the root sheet, which `ResolveItem` cannot see) + is existence-checked via `SCHEMATIC::ResolveItem(uuid)`. Missing → picker dropped + (`not_found` → `wxLogWarning`, no modal). Present-but-different-pointer (a remote + apply replaced the object, same uuid) → the picker is **re-anchored** + (`SetPickedItem`) and the restore targets the current live item. Without this, + undoing an entry whose item a remote apply freed dereferenced a dangling pointer. +- `eeschema/sch_commit.cpp` — `RecalculateConnections` moved **out** of the + `!( aCommitFlags & SKIP_UNDO )` gate in `pushSchEdit`: a commit that skips undo + still changes the model, and remote applies rely on the recalc (it was the whole + point of applying through real commits). `SaveCopyInUndoList` stays gated. +- `pcbnew/undo_redo.cpp` — the "Incomplete undo/redo operation" `wxMessageBox` + downgraded to `wxLogWarning`: dropped entries are routine in a collab session, + and a blocking modal would hang the wasm modal pump. + +### wasm bindings + +- `eeschema_embind.cpp` / `pcbnew_embind.cpp` — all four remote-apply Push sites + (`doApply` + `doApplyItems` per editor) now push with **`SKIP_UNDO`**. Change + detection is unaffected: emit suppression is keyed off `s_applyingRemote`, not + undo entries. +- **Removed-item ownership**: under `SKIP_UNDO` no undo picker takes ownership of + removed items (and the commit deletes the clone image), so the bindings free the + detached items after Push — both the explicit `removed[]` uuids and the upsert's + remove-old-before-re-add. Fields are excluded (CHT_REMOVE hides them; they stay + owned by their parent). Freeing stays in the binding layer, NOT the fork commit + classes, because existing `SKIP_UNDO` callers (DRC marker flows) manage their + removed items' lifetimes themselves — freeing in the commit would double-free. +- Test hooks `kicadCollabTestUndo` (runs `ACTIONS::undo` on the main-loop/fiber + stack) and `kicadCollabTestUndoDepth`, registered per-editor **and** in + `kicad_editor_embind.cpp`'s dispatcher — the merged image compiles out the + per-app `EMSCRIPTEN_BINDINGS` registrations, so a shared-name hook that is not + in the dispatcher silently vanishes from kicad_editor (build even forces the + relink; it's the registration that's conditional). + +## Coverage — tests/kicad/collab-undo.spec.ts (5/5) + +| Scenario | Editors | +|---|---| +| Remote apply adds no undo entry; undo reverts own op, peer's delete survives | eeschema + pcbnew | +| Stranded CHANGED entry (remote replaced the item) — undo re-anchors by uuid, no crash | eeschema | +| Stranded entry (remote deleted the item) — picker dropped quietly (log, not modal), no resurrect | eeschema + pcbnew | + +Pre-fix, the stranded cases dereferenced freed memory (eeschema had no guard at +all; pcbnew guarded existence but popped a blocking modal). + +## Accepted semantics (per doc 19) + +- Undo restores the full item image → a peer's concurrent edit to *another field of + the same item* is clobbered (LWW, converges; same trade-off as opt 14 granularity). +- Duplicate-KIID edge (local delete parked on undo + peer re-adds same uuid + undo) + remains untested; the differ keys by uuid so it should reconcile — follow-up test. +- Pre-existing local entries that a big adopt strands are dropped on first undo. diff --git a/kicad b/kicad index 81f9cd8..ca88773 160000 --- a/kicad +++ b/kicad @@ -1 +1 @@ -Subproject commit 81f9cd80fdf9ce91c41776c8d3187ba7d8807ad6 +Subproject commit ca8877324ce78d1bc9cb32bf634f091e0e76fb29 diff --git a/tests/kicad/collab-undo.spec.ts b/tests/kicad/collab-undo.spec.ts new file mode 100644 index 0000000..a1a7834 --- /dev/null +++ b/tests/kicad/collab-undo.spec.ts @@ -0,0 +1,314 @@ +import type { Page } from "@playwright/test"; +import { test, expect } from "./fixtures"; + +/** + * Miss 09 — collab-aware undo (docs/features/ysync-review/09 + 19): remote + * applies are pushed with SKIP_UNDO, so a peer's edit never lands on the local + * undo stack; Ctrl+Z is local-ops-only. Stale local undo entries — an item a + * remote apply replaced (same uuid, new object) or deleted — are re-anchored / + * dropped by the UUID guard at undo time instead of dereferencing a freed + * pointer. + * + * Single-tab, items-bridge driving style: kicadCollabApplyItems plays the + * remote peer, kicadCollabTest* plays the local user. Assertions are + * model-level (undo depth + positions); hasAbort pins crash-freedom, which is + * the point of the stranded-entry cases. + * + * Skips (not fails) on a wasm build that predates the undo test hooks. + */ + +const WIRE1 = "22222222-0000-0000-0000-000000000001"; +const WIRE2 = "22222222-0000-0000-0000-000000000002"; +const SAMPLE_SCH = `(kicad_sch +\t(version 20250114) +\t(generator "eeschema") +\t(generator_version "9.0") +\t(uuid "11111111-1111-1111-1111-111111111111") +\t(paper "A4") +\t(lib_symbols) +\t(wire (pts (xy 50.8 50.8) (xy 101.6 50.8)) (stroke (width 0) (type default)) (uuid "${WIRE1}")) +\t(wire (pts (xy 50.8 76.2) (xy 101.6 76.2)) (stroke (width 0) (type default)) (uuid "${WIRE2}")) +\t(sheet_instances (path "/" (page "1"))) +) +`; + +const VIA1 = "77777777-0000-0000-0000-000000000001"; +const SEG1 = "88888888-0000-0000-0000-000000000001"; +const SEG2 = "88888888-0000-0000-0000-000000000002"; +const SAMPLE_PCB = `(kicad_pcb +\t(version 20241229) +\t(generator "pcbnew") +\t(generator_version "9.0") +\t(general (thickness 1.6)) +\t(paper "A4") +\t(layers +\t\t(0 "F.Cu" signal) +\t\t(2 "B.Cu" signal) +\t\t(25 "Edge.Cuts" user) +\t) +\t(setup) +\t(net 0 "") +\t(via (at 80 80) (size 1.4) (drill 0.6) (layers "F.Cu" "B.Cu") (net 0) (uuid "${VIA1}")) +\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}")) +) +`; + +type FS = { mkdirTree(p: string): void; writeFile(p: string, d: string): void }; +type Mod = { + kicadOpenFile(p: string): unknown; + kicadCollabSnapshotItems(): string; + kicadCollabApplyItems(j: string): unknown; + kicadCollabTestMoveFirst(dx: number, dy: number): string; + kicadCollabTestRotateItem(id: string, deg: number): boolean; + kicadCollabGetPos(id: string): string; + kicadCollabTestUndo(): boolean; + kicadCollabTestUndoDepth(): number; +}; + +const BOOT_TIMEOUT = 150000; + +function hasAbort(l: { consoleLogs: string[]; errors: string[] }): boolean { + return [...l.consoleLogs, ...l.errors].some((s) => s.includes("Aborted(")); +} + +async function bootOpen(page: Page, url: string, content: string, file: string): Promise { + await page.goto(url); + await expect(page.locator("#canvas")).toBeVisible({ timeout: BOOT_TIMEOUT }); + await page.waitForFunction(() => !!window.wxElementRegistry, null, { timeout: BOOT_TIMEOUT }); + await page.waitForFunction( + () => { + const m = (window as unknown as { Module?: Mod }).Module; + return ( + typeof m?.kicadOpenFile === "function" && + typeof m?.kicadCollabSnapshotItems === "function" && + typeof m?.kicadCollabApplyItems === "function" && + typeof m?.kicadCollabTestMoveFirst === "function" + ); + }, + null, + { timeout: BOOT_TIMEOUT }, + ); + await page.waitForFunction( + () => + !!window.wxElementRegistry && + window.wxElementRegistry + .findAll({ visible: true }) + .some((e) => /Frame$/.test(e.typeName) || (e.name || "").endsWith("Frame")), + null, + { timeout: BOOT_TIMEOUT }, + ); + await page.evaluate( + ({ content, file }) => { + const w = window as unknown as { FS: FS; Module: Mod }; + try { + w.FS.mkdirTree("/home/kicad/documents"); + } catch { + /* exists */ + } + const p = `/home/kicad/documents/${file}`; + w.FS.writeFile(p, content); + w.Module.kicadOpenFile(p); + }, + { content, file }, + ); + + // Hook-presence guard: false ⇒ the wasm build predates the undo test hooks. + return page.evaluate(() => { + const m = (window as unknown as { Module: Record }).Module; + return typeof m.kicadCollabTestUndo === "function" && typeof m.kicadCollabTestUndoDepth === "function"; + }); +} + +const getPos = (page: Page, id: string) => + page.evaluate((i) => window.Module.kicadCollabGetPos(i), id); +const undoDepth = (page: Page) => page.evaluate(() => (window.Module as unknown as Mod).kicadCollabTestUndoDepth()); +const runUndo = (page: Page) => page.evaluate(() => (window.Module as unknown as Mod).kicadCollabTestUndo()); +const applyItems = (page: Page, wire: object) => + page.evaluate((j) => window.Module.kicadCollabApplyItems(j), JSON.stringify(wire)); + +test.describe("eeschema collab undo (miss 09: local-ops-only)", () => { + test.describe.configure({ timeout: 420000 }); + + test("remote apply adds no undo entry; undo reverts own op, not the peer's", async ({ + page, + testLogger, + }) => { + const hooked = await bootOpen(page, "/kicad/eeschema.html", SAMPLE_SCH, "undoA.kicad_sch"); + test.skip(!hooked, "wasm build predates the undo test hooks"); + + await page.evaluate(() => window.Module.kicadCollabSnapshotItems()); + expect(await undoDepth(page)).toBe(0); + + const orig: Record = { + [WIRE1]: await getPos(page, WIRE1), + [WIRE2]: await getPos(page, WIRE2), + }; + + // Local op → exactly one undo entry. + const movedId = (await page.evaluate(() => + window.Module.kicadCollabTestMoveFirst(200000, 0), + )) as string; + expect([WIRE1, WIRE2]).toContain(movedId); + await expect + .poll(() => getPos(page, movedId), { timeout: 15000, intervals: [250] }) + .not.toBe(orig[movedId]); + expect(await undoDepth(page)).toBe(1); + + // Remote apply (peer deletes the other wire) → depth must NOT grow. + const target = movedId === WIRE1 ? WIRE2 : WIRE1; + await applyItems(page, { added: [], changed: [], removed: [target] }); + await expect.poll(() => getPos(page, target), { timeout: 15000, intervals: [250] }).toBe(""); + expect(await undoDepth(page), "remote apply must not land on the undo stack").toBe(1); + + // Undo → own move reverts, peer's delete stays. + await runUndo(page); + await expect + .poll(() => getPos(page, movedId), { timeout: 15000, intervals: [250] }) + .toBe(orig[movedId]); + expect(await getPos(page, target), "undo must not resurrect the peer's delete").toBe(""); + await expect.poll(() => undoDepth(page), { timeout: 15000, intervals: [250] }).toBe(0); + expect(hasAbort(testLogger), "no WASM abort").toBe(false); + }); + + test("stranded CHANGED entry: remote replaced the item — undo re-anchors by uuid", async ({ + page, + testLogger, + }) => { + const hooked = await bootOpen(page, "/kicad/eeschema.html", SAMPLE_SCH, "undoB.kicad_sch"); + test.skip(!hooked, "wasm build predates the undo test hooks"); + + // Original WIRE1 blob (pre-edit geometry) — the "peer's" replacement payload. + const snap = JSON.parse( + await page.evaluate(() => window.Module.kicadCollabSnapshotItems()), + ) as { added: Array<{ id?: string; sexpr?: string }> }; + const wire1Blob = snap.added.find((e) => e.id === WIRE1 || (e.sexpr ?? "").includes(WIRE1)); + expect(wire1Blob?.sexpr, "snapshot must carry WIRE1's blob").toBeTruthy(); + + // Local op referencing WIRE1 → undo entry holds a pointer to today's object. + await page.evaluate( + (id) => + (window as unknown as { Module: { kicadCollabTestRotateItem(i: string, d: number): boolean } }) + .Module.kicadCollabTestRotateItem(id, 90), + WIRE1, + ); + await expect.poll(() => undoDepth(page), { timeout: 15000, intervals: [250] }).toBe(1); + + // Remote upsert of WIRE1 (remove old object + re-add same uuid) frees the + // pointer the undo entry holds; WIRE2's removal doubles as the completion marker. + await applyItems(page, { + added: [], + changed: [{ sexpr: wire1Blob!.sexpr }], + removed: [WIRE2], + }); + await expect.poll(() => getPos(page, WIRE2), { timeout: 15000, intervals: [250] }).toBe(""); + expect(await undoDepth(page)).toBe(1); + + // Undo → the guard re-resolves WIRE1 by uuid onto the new live object. The + // assertion that matters is crash-freedom (pre-guard this dereferenced a + // freed pointer). + await runUndo(page); + await expect.poll(() => undoDepth(page), { timeout: 15000, intervals: [250] }).toBe(0); + expect(await getPos(page, WIRE1), "WIRE1 must still exist after undo").not.toBe(""); + expect(hasAbort(testLogger), "no WASM abort").toBe(false); + }); + + test("stranded entry: remote deleted the item — undo drops the picker quietly", async ({ + page, + testLogger, + }) => { + const hooked = await bootOpen(page, "/kicad/eeschema.html", SAMPLE_SCH, "undoC.kicad_sch"); + test.skip(!hooked, "wasm build predates the undo test hooks"); + + await page.evaluate(() => window.Module.kicadCollabSnapshotItems()); + + await page.evaluate( + (id) => + (window as unknown as { Module: { kicadCollabTestRotateItem(i: string, d: number): boolean } }) + .Module.kicadCollabTestRotateItem(id, 90), + WIRE1, + ); + await expect.poll(() => undoDepth(page), { timeout: 15000, intervals: [250] }).toBe(1); + + await applyItems(page, { added: [], changed: [], removed: [WIRE1] }); + await expect.poll(() => getPos(page, WIRE1), { timeout: 15000, intervals: [250] }).toBe(""); + + // Undo → the entry's item is gone; the guard drops the picker (log, no modal, no crash). + await runUndo(page); + await expect.poll(() => undoDepth(page), { timeout: 15000, intervals: [250] }).toBe(0); + expect(await getPos(page, WIRE1), "undo must not resurrect the deleted item").toBe(""); + expect(hasAbort(testLogger), "no WASM abort").toBe(false); + }); +}); + +test.describe("pcbnew collab undo (miss 09: local-ops-only)", () => { + test.describe.configure({ timeout: 420000 }); + + test("remote apply adds no undo entry; undo reverts own op, not the peer's", async ({ + page, + testLogger, + }) => { + const hooked = await bootOpen(page, "/kicad/pcbnew-collab.html", SAMPLE_PCB, "undoA.kicad_pcb"); + test.skip(!hooked, "wasm build predates the undo test hooks"); + + await page.evaluate(() => window.Module.kicadCollabSnapshotItems()); + expect(await undoDepth(page)).toBe(0); + + const ids = [VIA1, SEG1, SEG2]; + const orig: Record = {}; + for (const id of ids) orig[id] = await getPos(page, id); + + const movedId = (await page.evaluate(() => + window.Module.kicadCollabTestMoveFirst(200000, 0), + )) as string; + expect(ids).toContain(movedId); + await expect + .poll(() => getPos(page, movedId), { timeout: 15000, intervals: [250] }) + .not.toBe(orig[movedId]); + expect(await undoDepth(page)).toBe(1); + + const target = movedId === SEG1 ? SEG2 : SEG1; + await applyItems(page, { added: [], changed: [], removed: [target] }); + await expect.poll(() => getPos(page, target), { timeout: 15000, intervals: [250] }).toBe(""); + expect(await undoDepth(page), "remote apply must not land on the undo stack").toBe(1); + + await runUndo(page); + await expect + .poll(() => getPos(page, movedId), { timeout: 15000, intervals: [250] }) + .toBe(orig[movedId]); + expect(await getPos(page, target), "undo must not resurrect the peer's delete").toBe(""); + await expect.poll(() => undoDepth(page), { timeout: 15000, intervals: [250] }).toBe(0); + expect(hasAbort(testLogger), "no WASM abort").toBe(false); + }); + + test("stranded entry: remote deleted the item — undo drops it (log, not modal)", async ({ + page, + testLogger, + }) => { + const hooked = await bootOpen(page, "/kicad/pcbnew-collab.html", SAMPLE_PCB, "undoB.kicad_pcb"); + test.skip(!hooked, "wasm build predates the undo test hooks"); + + await page.evaluate(() => window.Module.kicadCollabSnapshotItems()); + + const origins: Record = {}; + for (const id of [VIA1, SEG1, SEG2]) origins[id] = await getPos(page, id); + + const movedId = (await page.evaluate(() => + window.Module.kicadCollabTestMoveFirst(200000, 0), + )) as string; + await expect + .poll(() => getPos(page, movedId), { timeout: 15000, intervals: [250] }) + .not.toBe(origins[movedId]); + await expect.poll(() => undoDepth(page), { timeout: 15000, intervals: [250] }).toBe(1); + + await applyItems(page, { added: [], changed: [], removed: [movedId] }); + await expect.poll(() => getPos(page, movedId), { timeout: 15000, intervals: [250] }).toBe(""); + + // Undo → the CHANGED picker's item is gone: the pcbnew guard drops it. This + // path used to raise a blocking wxMessageBox; now it must only log. + await runUndo(page); + await expect.poll(() => undoDepth(page), { timeout: 15000, intervals: [250] }).toBe(0); + expect(await getPos(page, movedId), "undo must not resurrect the deleted item").toBe(""); + expect(hasAbort(testLogger), "no WASM abort").toBe(false); + }); +}); diff --git a/wasm/bindings/eeschema_embind.cpp b/wasm/bindings/eeschema_embind.cpp index 573e7bd..5f4d88e 100644 --- a/wasm/bindings/eeschema_embind.cpp +++ b/wasm/bindings/eeschema_embind.cpp @@ -1064,6 +1064,11 @@ void doApply( SCH_EDIT_FRAME* aFrame, const json& aDelta ) SCH_COMMIT commit( aFrame ); bool staged = false; + // With SKIP_UNDO no undo picker takes ownership of removed items; the commit + // detaches them from the screen and we free them after Push. Fields are hidden + // by CHT_REMOVE, not detached (still owned by their parent), so never freed. + std::vector removedItems; + for( const json& rid : aDelta.value( "removed", json::array() ) ) { SCH_SHEET_PATH path; @@ -1072,6 +1077,10 @@ void doApply( SCH_EDIT_FRAME* aFrame, const json& aDelta ) if( SCH_ITEM* item = sch.ResolveItem( id, &path, /*allowNull*/ true ) ) { commit.Remove( item, path.LastScreen() ); + + if( item->Type() != SCH_FIELD_T ) + removedItems.push_back( item ); + staged = true; } } @@ -1120,8 +1129,14 @@ void doApply( SCH_EDIT_FRAME* aFrame, const json& aDelta ) } } + // SKIP_UNDO: a peer's edit must never land on this editor's undo stack — Ctrl+Z + // would revert (and re-broadcast) the peer's work. Undo is local-ops-only; stale + // local undo entries are dropped/re-resolved by UUID at undo time (miss 09). if( staged ) - commit.Push( wxT( "Collaborative edit" ) ); + commit.Push( wxT( "Collaborative edit" ), SKIP_UNDO ); + + for( SCH_ITEM* item : removedItems ) + delete item; // The applied remote changes (and any connectivity cleanup they triggered) are now the // shared state — fold them into the baseline so the post-apply listener flush doesn't @@ -1146,6 +1161,10 @@ void doApplyItems( SCH_EDIT_FRAME* aFrame, const json& aWire ) std::vector touched; // uuids this apply acts on (targeted rebaseline) + // Owned by nobody once the SKIP_UNDO commit detaches them — freed after Push + // (fields are hidden, not detached, so excluded). See doApply. + std::vector removedItems; + for( const json& rid : aWire.value( "removed", json::array() ) ) { SCH_SHEET_PATH path; @@ -1156,6 +1175,10 @@ void doApplyItems( SCH_EDIT_FRAME* aFrame, const json& aWire ) if( SCH_ITEM* item = sch.ResolveItem( id, &path, /*allowNull*/ true ) ) { commit.Remove( item, path.LastScreen() ); + + if( item->Type() != SCH_FIELD_T ) + removedItems.push_back( item ); + staged = true; } } @@ -1223,8 +1246,13 @@ void doApplyItems( SCH_EDIT_FRAME* aFrame, const json& aWire ) SCH_SHEET_PATH path; if( SCH_ITEM* existing = sch.ResolveItem( item->m_Uuid, &path, /*allowNull*/ true ) ) + { commit.Remove( existing, path.LastScreen() ); + if( existing->Type() != SCH_FIELD_T ) + removedItems.push_back( existing ); + } + if( item->Type() == SCH_SYMBOL_T ) { auto* sym = static_cast( item ); @@ -1245,8 +1273,12 @@ void doApplyItems( SCH_EDIT_FRAME* aFrame, const json& aWire ) for( const json& w : aWire.value( "changed", json::array() ) ) upsert( w ); + // SKIP_UNDO: remote applies never land on the local undo stack (see doApply). if( staged ) - commit.Push( wxT( "Collaborative edit (items)" ) ); + commit.Push( wxT( "Collaborative edit (items)" ), SKIP_UNDO ); + + for( SCH_ITEM* item : removedItems ) + delete item; // Fold ONLY the applied uuids into the baseline (echo suppression), then flush: // anything else that now differs — a concurrent local edit, the connectivity @@ -1505,6 +1537,35 @@ bool schCollabTestRotateItem( std::string aId, double aDeg ) return true; } +// Run Edit>Undo exactly like the UI would (main-loop + fiber stack) — miss 09: +// exercises the local-ops-only undo policy and the stale-picker UUID guard. +bool schCollabTestUndo() +{ + SCH_EDIT_FRAME* fr = schFrame(); + + if( !fr ) + return false; + + fr->CallAfter( [fr]() { + COROUTINE cor( [fr]( int ) -> int + { + fr->GetToolManager()->RunAction( ACTIONS::undo ); + return 0; + } ); + cor.Call( 0 ); + } ); + + return true; +} + +// Local undo stack depth — remote applies must not grow it (miss 09). +int schCollabTestUndoDepth() +{ + SCH_EDIT_FRAME* fr = schFrame(); + + return fr ? fr->GetUndoCommandCount() : -1; +} + // Set a symbol's Value field text — bug 04: fields live inside the symbol (not // in screen->Items()) and the symbol json carries no field text, so the most // common schematic edit after moving things never syncs. @@ -2116,6 +2177,8 @@ EMSCRIPTEN_BINDINGS(eeschema) { // ysync-review repro hooks shared with pcbnew (dispatched when merged). function("kicadCollabTestRemoveItem", &schCollabTestRemoveItem); function("kicadCollabTestRotateItem", &schCollabTestRotateItem); + function("kicadCollabTestUndo", &schCollabTestUndo); + function("kicadCollabTestUndoDepth", &schCollabTestUndoDepth); // Presence (collab-presence 0003) — shared names with pcbnew's 0002 set. function("kicadCollabPresenceStart", &schCollabPresenceStart); function("kicadCollabSetRemote", &schCollabSetRemote); diff --git a/wasm/bindings/kicad_editor_embind.cpp b/wasm/bindings/kicad_editor_embind.cpp index 2ce7c8d..ce4c01c 100644 --- a/wasm/bindings/kicad_editor_embind.cpp +++ b/wasm/bindings/kicad_editor_embind.cpp @@ -45,6 +45,9 @@ std::string pcbCollabTestMoveFirst( int aDx, int aDy ); std::string pcbCollabGetPos( std::string aId ); bool pcbCollabTestRemoveItem( std::string aId ); bool pcbCollabTestRotateItem( std::string aId, double aDeg ); +// Collab-aware undo (ysync miss 09). +bool pcbCollabTestUndo(); +int pcbCollabTestUndoDepth(); // Presence (collab-presence 0002) + comment pins/panning (0005). void pcbCollabPresenceStart(); void pcbCollabSetRemote( std::string aJson ); @@ -74,6 +77,9 @@ std::string schCollabTestMoveFirst( int aDx, int aDy ); std::string schCollabGetPos( std::string aId ); bool schCollabTestRemoveItem( std::string aId ); bool schCollabTestRotateItem( std::string aId, double aDeg ); +// Collab-aware undo (ysync miss 09). +bool schCollabTestUndo(); +int schCollabTestUndoDepth(); // Presence (collab-presence 0003 — eeschema counterparts) + pins (0005). void schCollabPresenceStart(); void schCollabSetRemote( std::string aJson ); @@ -171,6 +177,16 @@ static bool collabTestRotateItem( std::string aId, double aDeg ) : schCollabTestRotateItem( aId, aDeg ); } +static bool collabTestUndo() +{ + return pcbEditorActive() ? pcbCollabTestUndo() : schCollabTestUndo(); +} + +static int collabTestUndoDepth() +{ + return pcbEditorActive() ? pcbCollabTestUndoDepth() : schCollabTestUndoDepth(); +} + // Presence shims (collab-presence 0002 pcbnew / 0003 eeschema): route to the live // editor's implementation, same pattern as the collab bridge shims above. static void collabPresenceStart() @@ -271,6 +287,9 @@ EMSCRIPTEN_BINDINGS(kicad_editor) { // endpoint, field text — flow from the per-editor blocks unchanged). function("kicadCollabTestRemoveItem", &collabTestRemoveItem); function("kicadCollabTestRotateItem", &collabTestRotateItem); + // Collab-aware undo (ysync miss 09). + function("kicadCollabTestUndo", &collabTestUndo); + function("kicadCollabTestUndoDepth", &collabTestUndoDepth); // Presence (collab-presence 0002/0003) + comment pins/panning (0005). function("kicadCollabPresenceStart", &collabPresenceStart); function("kicadCollabSetRemote", &collabSetRemote); diff --git a/wasm/bindings/pcbnew_embind.cpp b/wasm/bindings/pcbnew_embind.cpp index 797dfc1..216678e 100644 --- a/wasm/bindings/pcbnew_embind.cpp +++ b/wasm/bindings/pcbnew_embind.cpp @@ -914,6 +914,10 @@ void doApply( PCB_EDIT_FRAME* aFrame, const json& aDelta ) BOARD_COMMIT commit( aFrame ); bool staged = false; + // With SKIP_UNDO no undo picker takes ownership of removed items; the commit + // detaches them from the board and we free them after Push. + std::vector removedItems; + for( const json& rid : aDelta.value( "removed", json::array() ) ) { KIID id( wxString::FromUTF8( rid.get().c_str() ) ); @@ -928,6 +932,7 @@ void doApply( PCB_EDIT_FRAME* aFrame, const json& aDelta ) continue; commit.Remove( item ); + removedItems.push_back( item ); staged = true; } } @@ -963,8 +968,14 @@ void doApply( PCB_EDIT_FRAME* aFrame, const json& aDelta ) } } + // SKIP_UNDO: a peer's edit must never land on this editor's undo stack — Ctrl+Z + // would revert (and re-broadcast) the peer's work. Undo is local-ops-only; stale + // local undo entries are dropped/re-resolved by UUID at undo time (miss 09). if( staged ) - commit.Push( wxT( "Collaborative edit" ) ); + commit.Push( wxT( "Collaborative edit" ), SKIP_UNDO ); + + for( BOARD_ITEM* item : removedItems ) + delete item; // The applied remote changes (and any connectivity cleanup they triggered) are now the // shared state — fold them into the baseline so the post-apply listener flush doesn't @@ -988,6 +999,9 @@ void doApplyItems( PCB_EDIT_FRAME* aFrame, const json& aWire ) std::vector touched; // root uuids this apply acts on (targeted rebaseline) + // Owned by nobody once the SKIP_UNDO commit detaches them — freed after Push. + std::vector removedItems; + std::set removedIds; for( const json& rid : aWire.value( "removed", json::array() ) ) @@ -1012,6 +1026,12 @@ void doApplyItems( PCB_EDIT_FRAME* aFrame, const json& aWire ) } commit.Remove( item ); + + // A bare child "removal" of a PCB_FIELD_T is a hide, not a detach — + // the field stays owned by its parent footprint. + if( item->Type() != PCB_FIELD_T ) + removedItems.push_back( item ); + staged = true; } } @@ -1046,6 +1066,7 @@ void doApplyItems( PCB_EDIT_FRAME* aFrame, const json& aWire ) existing = fp; commit.Remove( existing ); + removedItems.push_back( existing ); } touched.push_back( toUtf8( parsed->m_Uuid.AsString() ) ); @@ -1059,8 +1080,12 @@ void doApplyItems( PCB_EDIT_FRAME* aFrame, const json& aWire ) for( const json& w : aWire.value( "changed", json::array() ) ) upsert( w ); + // SKIP_UNDO: remote applies never land on the local undo stack (see doApply). if( staged ) - commit.Push( wxT( "Collaborative edit (items)" ) ); + commit.Push( wxT( "Collaborative edit (items)" ), SKIP_UNDO ); + + for( BOARD_ITEM* item : removedItems ) + delete item; // Fold ONLY the applied uuids into the baseline (echo suppression), then flush: // anything else that now differs — a concurrent local edit, cleanup this apply's @@ -2235,6 +2260,35 @@ static BOARD_ITEM* testResolve( PCB_EDIT_FRAME* aFrame, const std::string& aId ) // Delete an item by uuid. With a footprint CHILD uuid this is the bug-03 // sending half (the UI's fp-text delete): the emit must lift to a parent // re-blob; today it goes out as a bare child removal. +// Run Edit>Undo exactly like the UI would (main-loop + fiber stack) — miss 09: +// exercises the local-ops-only undo policy and the stale-picker UUID guard. +bool pcbCollabTestUndo() +{ + PCB_EDIT_FRAME* fr = pcbFrame(); + + if( !fr ) + return false; + + fr->CallAfter( [fr]() { + COROUTINE cor( [fr]( int ) -> int + { + fr->GetToolManager()->RunAction( ACTIONS::undo ); + return 0; + } ); + cor.Call( 0 ); + } ); + + return true; +} + +// Local undo stack depth — remote applies must not grow it (miss 09). +int pcbCollabTestUndoDepth() +{ + PCB_EDIT_FRAME* fr = pcbFrame(); + + return fr ? fr->GetUndoCommandCount() : -1; +} + bool pcbCollabTestRemoveItem( std::string aId ) { PCB_EDIT_FRAME* fr = pcbFrame(); @@ -2436,6 +2490,8 @@ EMSCRIPTEN_BINDINGS(pcbnew) { function("kicadCollabGetPos", &pcbCollabGetPos); // ysync-review repro hooks shared with eeschema (dispatched when merged). function("kicadCollabTestRemoveItem", &pcbCollabTestRemoveItem); + function("kicadCollabTestUndo", &pcbCollabTestUndo); + function("kicadCollabTestUndoDepth", &pcbCollabTestUndoDepth); function("kicadCollabTestRotateItem", &pcbCollabTestRotateItem); // Presence (collab-presence 0002) — shared names; eeschema's counterparts land // with 0003 (the merged image dispatches pcb-only until then).