diff --git a/tests/apps/Makefile.wasm b/tests/apps/Makefile.wasm index d49d278..0b49cfe 100644 --- a/tests/apps/Makefile.wasm +++ b/tests/apps/Makefile.wasm @@ -180,6 +180,7 @@ all: minimal_test.html \ $(S)/dialog/dialog_test.html \ $(S)/timer/timer_test.html \ $(S)/tree/tree_test.html \ + $(S)/tree-hier/tree_hier_test.html \ $(S)/radiogroups/radiogroups_test.html \ $(S)/dataview/dataview_test.html \ $(S)/htmlwin/htmlwin_test.html \ @@ -400,6 +401,13 @@ $(S)/tree/tree_test.o: $(S)/tree/tree_test.cpp $(S)/tree/tree_test.html: $(S)/tree/tree_test.o $(WX_CORE_LIB) $(JS_FILES) $(CXX) $< $(LDFLAGS_NOGL) --pre-js $(JS) --shell-file $(HTML) -o $@ +# Hierarchy-pane-like tree test (no GL): hidden root + images + bold item +$(S)/tree-hier/tree_hier_test.o: $(S)/tree-hier/tree_hier_test.cpp + $(CXX) -c $(CXXFLAGS) $< -o $@ + +$(S)/tree-hier/tree_hier_test.html: $(S)/tree-hier/tree_hier_test.o $(WX_CORE_LIB) $(JS_FILES) + $(CXX) $< $(LDFLAGS_NOGL) --pre-js $(JS) --shell-file $(HTML) -o $@ + # Radio button groups test (no GL) - multiple wxRB_GROUP groups in one window $(S)/radiogroups/radiogroups_test.o: $(S)/radiogroups/radiogroups_test.cpp $(CXX) -c $(CXXFLAGS) $< -o $@ @@ -808,6 +816,7 @@ grid: $(S)/grid/grid_test.html dialog: $(S)/dialog/dialog_test.html timer: $(S)/timer/timer_test.html tree: $(S)/tree/tree_test.html +tree-hier: $(S)/tree-hier/tree_hier_test.html radiogroups: $(S)/radiogroups/radiogroups_test.html dataview: $(S)/dataview/dataview_test.html htmlwin: $(S)/htmlwin/htmlwin_test.html diff --git a/tests/apps/standalone/tree-hier/tree_hier_test.cpp b/tests/apps/standalone/tree-hier/tree_hier_test.cpp new file mode 100644 index 0000000..d214835 --- /dev/null +++ b/tests/apps/standalone/tree-hier/tree_hier_test.cpp @@ -0,0 +1,80 @@ +// wxTreeCtrl configured like eeschema's HIERARCHY_PANE: hidden root, buttons, +// per-item images (wxBitmapBundle), bold "current sheet" item, no explicit lines. +// Repro for the mangled hierarchy pane in the wasm port (icon/text misaligned, +// oversized rows, black text on the selection highlight). +#include +#ifndef WX_PRECOMP +#include +#endif +#include +#include +#include +#ifdef __EMSCRIPTEN__ +#include +#endif + +static wxBitmap MakeDot(int size, const wxColour& fill) +{ + wxBitmap bmp(size, size, 32); + bmp.UseAlpha(); + wxMemoryDC dc(bmp); + dc.SetBackground(*wxTRANSPARENT_BRUSH); + dc.Clear(); + dc.SetPen(wxPen(fill)); + dc.SetBrush(wxBrush(fill)); + dc.DrawEllipse(1, 1, size - 2, size - 2); + dc.SelectObject(wxNullBitmap); + return bmp; +} + +class HierFrame : public wxFrame +{ +public: + HierFrame() : wxFrame(nullptr, wxID_ANY, "Hierarchy tree test", wxDefaultPosition, wxSize(420, 420)) + { + wxBoxSizer* sizer = new wxBoxSizer(wxVERTICAL); + m_tree = new wxTreeCtrl(this, wxID_ANY, wxDefaultPosition, wxDefaultSize, + wxTR_HAS_BUTTONS | wxTR_EDIT_LABELS | wxTR_HIDE_ROOT); + wxVector images; + // KiCad's tree_nosel/tree_sel are 16px dots (bundle carries 16 + 32 variants). + images.push_back(wxBitmapBundle::FromBitmaps(MakeDot(16, wxColour(160, 160, 160)), + MakeDot(32, wxColour(160, 160, 160)))); + images.push_back(wxBitmapBundle::FromBitmaps(MakeDot(16, wxColour(40, 40, 40)), + MakeDot(32, wxColour(40, 40, 40)))); + m_tree->SetImages(images); + + wxTreeItemId root = m_tree->AddRoot("root"); + wxTreeItemId top = m_tree->AppendItem(root, "Arduino Leonardo (page 1)", 0, 1); + m_tree->SetItemBold(top, true); + m_tree->AppendItem(top, "Headers (page 2)", 0, 1); + m_tree->AppendItem(top, "Power (page 3)", 0, 1); + wxTreeItemId mcu = m_tree->AppendItem(top, "ATMEGA32U4-AU (page 4)", 0, 1); + m_tree->AppendItem(mcu, "USB (page 5)", 0, 1); + m_tree->ExpandAll(); + m_tree->SelectItem(top); + sizer->Add(m_tree, 1, wxEXPAND); + + wxBoxSizer* btns = new wxBoxSizer(wxHORIZONTAL); + wxButton* focusBtn = new wxButton(this, wxID_ANY, "Take focus"); + btns->Add(focusBtn, 0, wxALL, 4); + sizer->Add(btns, 0); + SetSizer(sizer); + Layout(); +#ifdef __EMSCRIPTEN__ + EM_ASM({ console.log('[TREE_HIER_TEST] started'); }); +#endif + } +private: + wxTreeCtrl* m_tree; +}; + +class HierApp : public wxApp +{ +public: + bool OnInit() override + { + (new HierFrame())->Show(); + return true; + } +}; +wxIMPLEMENT_APP(HierApp); diff --git a/tests/e2e/tree-hier.spec.ts b/tests/e2e/tree-hier.spec.ts new file mode 100644 index 0000000..bbe8350 --- /dev/null +++ b/tests/e2e/tree-hier.spec.ts @@ -0,0 +1,59 @@ +import { test, expect } from './utils/fixtures'; +import { findAllTreeItems, waitForWxApp, stableShot } from './utils/element-tracker'; + +/** + * eeschema's Schematic Hierarchy pane = wxTreeCtrl with a hidden root, a + * per-item wxBitmapBundle (16px dots) and a bold "current sheet". Regression + * for two port bugs: the bundle's preferred LOGICAL size was reported at the + * device-pixel size (32 on retina) so every row reserved twice the icon + * height with the icon pinned to the row top; and an unfocused selection was + * drawn black-on-highlight (wxSYS_COLOUR_LISTBOXHIGHLIGHTTEXT defaulted to the + * window text colour while the generic renderer always paints HIGHLIGHT). + */ +test.describe('wxTreeCtrl hierarchy-pane configuration', () => { + test.beforeEach(async ({ page }) => { + await page.goto('/standalone/tree-hier/tree_hier_test.html'); + await waitForWxApp(page); + }); + + test('rows with 16px bundle icons stay text-height, not icon@2x-height', async ({ page }) => { + const items = await findAllTreeItems(page); + expect(items.length).toBe(5); + const byY = [...items].sort((a, b) => a.screenY - b.screenY); + // 10pt text ≈ 16px + 2px padding + 2px spacing; a 32px "logical" icon made this ~35. + for (const it of byY) expect(it.height, `row "${it.label}"`).toBeLessThanOrEqual(24); + const pitches = byY.slice(1).map((it, i) => it.screenY - byY[i].screenY); + for (const p of pitches) expect(p).toBeLessThanOrEqual(24); + await stableShot(page, 'tree-hier-01-loaded.png'); + }); + + test('an unfocused selection is drawn light-on-highlight, not black-on-blue', async ({ page }) => { + const items = await findAllTreeItems(page); + const sel = items.find(it => it.label.startsWith('Arduino Leonardo'))!; + expect(sel).toBeDefined(); + // Sample the label row at device resolution: the highlight (navy) must be + // present and the glyph pixels on it must be light, never near-black. + const dpr = await page.evaluate(() => window.devicePixelRatio); + const px = await page.evaluate( + ({ x, y, w, h, dpr }) => { + const canvas = document.querySelector('canvas') as HTMLCanvasElement; + const ctx = canvas.getContext('2d')!; + const r = canvas.getBoundingClientRect(); + const d = ctx.getImageData((x - r.x) * dpr, (y - r.y) * dpr, w * dpr, h * dpr).data; + let navy = 0, dark = 0, light = 0; + for (let i = 0; i < d.length; i += 4) { + const [R, G, B] = [d[i], d[i + 1], d[i + 2]]; + if (B > 90 && R < 40 && G < 40) navy++; + else if (R < 60 && G < 60 && B < 60) dark++; + else if (R > 200 && G > 200 && B > 200) light++; + } + return { navy, dark, light, total: d.length / 4 }; + }, + // Skip the leading 16px item icon (the "selected" dot is dark by design). + { x: sel.screenX + 22, y: sel.screenY, w: sel.width - 22, h: sel.height, dpr } + ); + expect(px.navy, 'highlight painted under the selected label').toBeGreaterThan(px.total * 0.2); + expect(px.light, 'light glyph pixels on the highlight').toBeGreaterThan(px.total * 0.02); + expect(px.dark, 'no black glyphs on the highlight').toBeLessThan(px.total * 0.01); + }); +}); diff --git a/web/pcbjam-shared b/web/pcbjam-shared index 4fd6af2..031faae 160000 --- a/web/pcbjam-shared +++ b/web/pcbjam-shared @@ -1 +1 @@ -Subproject commit 4fd6af24a2a8d818970931bbf13d15c9818e6169 +Subproject commit 031faaedd37365ab7391707c2e61463b0f842d9d diff --git a/web/standalone/src/components/wasm-tool/collab-start.ts b/web/standalone/src/components/wasm-tool/collab-start.ts index 9496d69..b9908fc 100644 --- a/web/standalone/src/components/wasm-tool/collab-start.ts +++ b/web/standalone/src/components/wasm-tool/collab-start.ts @@ -4,6 +4,7 @@ import { docToFile, fileToDoc, ydocHasState, + ydocIsHollow, yToDoc, type KicadDoc, type Tool, @@ -87,7 +88,7 @@ export async function maybeConnectDocSession( // populated drawing sheet (pl_editor `.kicad_wks`) has zero uuid items, so an // items-only check makes a joining tab refetch the stale file instead of // materializing the shared doc's current state. - if (!ydocHasState(session.doc)) { + if (!ydocHasState(session.doc) || ydocIsHollow(session.doc)) { opts.log(`[ydoc] room ${room} is empty — falling back to the API fetch (will file-seed)`); return { session }; } diff --git a/web/standalone/src/wasm/collab/kicad-binding.test.ts b/web/standalone/src/wasm/collab/kicad-binding.test.ts index da1bf7c..4698f20 100644 --- a/web/standalone/src/wasm/collab/kicad-binding.test.ts +++ b/web/standalone/src/wasm/collab/kicad-binding.test.ts @@ -9,7 +9,9 @@ import { renderItem, SEXPR_VERSION_CURRENT, sexprToItems, + syncLayoutToY, ydocHasState, + ydocIsHollow, ydocSexprVersion, yToDoc, type KicadItem, @@ -198,6 +200,49 @@ describe("bindKicadCollab — two editors over relayed Y.Docs", () => { expect(edB.store["fp-1"]).toBeDefined(); // adopted }); + it("a HOLLOW room (layout only from a save-all sync, never seeded) is file-seeded, NOT adopted", () => { + // Save-all fired the layout sync into a room nobody had entered: the doc got + // meta/layout but zero items and no seedNonce. Adopting it removed every + // item on the subsheet the editor had just shown (arduino subsheet blank bug). + const { a, edA, edB, bindA, bindB } = setup(); + const file = `(kicad_wks (version 20220228) (generator "pl_editor") + (setup (textsize 1.5 1.5) (linewidth 0.15)) + (rect (uuid "r-1") (name "border") (start 0 0 ltcorner) (end 0 0 rbcorner)) +) +`; + const seedDoc = fileToDoc(file); + syncLayoutToY(seedDoc, a, "layout-save"); // the hollow footprint + expect(ydocHasState(a)).toBe(true); + expect(ydocIsHollow(a)).toBe(true); + + Object.assign(edA.store, seedDoc.items); // editor shows the file + bindA.seed(seedDoc); + + expect(edA.applied.length).toBe(0); // nothing removed from the editor + expect(edA.store["r-1"]).toBeDefined(); + expect(ydocIsHollow(a)).toBe(false); // healed: items + seed marker in the doc + expect(docToFile(yToDoc(a))).toBe(docToFile(seedDoc)); + bindB.seed(); // a peer joining now adopts the real content + expect(edB.store["r-1"]).toBeDefined(); + }); + + it("a seeded room that was legitimately emptied is still adopted (not mistaken for hollow)", () => { + const { a, edA, edB, bindA, bindB } = setup(); + const file = `(kicad_wks (version 20220228) (generator "pl_editor") + (rect (uuid "r-1") (name "border") (start 0 0 ltcorner) (end 0 0 rbcorner)) +) +`; + const seedDoc = fileToDoc(file); + Object.assign(edA.store, seedDoc.items); + bindA.seed(seedDoc); // file-seeded → seed marker present + edA.localRemove("r-1"); // the peer deletes everything on the sheet + expect(ydocHasState(a)).toBe(true); + expect(ydocIsHollow(a)).toBe(false); // seeded, merely empty + Object.assign(edB.store, seedDoc.items); // B cold-opens the stale file + bindB.seed(seedDoc); + expect(edB.store["r-1"]).toBeUndefined(); // doc authority: the deletion wins + }); + it("pre-seed remote state does NOT stream into the editor (adopt covers it)", () => { const { edA, edB, bindA, bindB } = setup(); seedEditor(edA, FP); diff --git a/web/standalone/src/wasm/collab/kicad-binding.ts b/web/standalone/src/wasm/collab/kicad-binding.ts index 6f0aa0a..bb3bc8e 100644 --- a/web/standalone/src/wasm/collab/kicad-binding.ts +++ b/web/standalone/src/wasm/collab/kicad-binding.ts @@ -20,6 +20,7 @@ import { Y_KDOC_REVERT_REASON, Y_KDOC_SEED_NONCE, ydocHasState, + ydocIsHollow, ydocSexprVersion, yToItemUnchecked, type ItemsWireDelta, @@ -264,7 +265,14 @@ export function bindKicadCollab( // `ydocHasState` (meta + layout + items), NOT `items.size`: a populated // drawing sheet (pl_editor .kicad_wks) has zero uuid items, so an items-only // check would mis-classify a seeded room as empty and re-seed/clobber it. - if (opts?.editorMatchesDoc && ydocHasState(doc)) { + // A HOLLOW doc (layout/meta, zero items, never seeded — a save-all's + // layout sync into a room nobody had entered) counts as empty here: + // adopting it would remove every item on the sheet the editor just + // showed (the "subsheet renders then goes blank" bug), and a file + // materialized from it is title-block-only. + const hollow = ydocIsHollow(doc); + if (hollow) clog("seed: doc is HOLLOW (layout only, never seeded) → treating as empty"); + if (opts?.editorMatchesDoc && ydocHasState(doc) && !hollow) { // The editor opened exactly this doc's content (Y.Doc-load path): no // adopt apply needed. snapshotItems() still runs to BASELINE the wasm // differ — otherwise the first local edit would re-emit the full model. @@ -276,7 +284,7 @@ export function bindKicadCollab( } return; } - if (!ydocHasState(doc) && seedDoc) { + if ((!ydocHasState(doc) || hollow) && seedDoc) { if (readOnly) { // A viewer never authors a room. The editor keeps showing the file it // opened; when a writer later seeds this room, the (now-open) UP @@ -336,7 +344,7 @@ export function bindKicadCollab( return; } - const hasState = ydocHasState(doc); + const hasState = ydocHasState(doc) && !hollow; if (!hasState) { if (readOnly) { @@ -349,6 +357,12 @@ export function bindKicadCollab( doc.transact(() => { applyDeltaToY(doc, local, ORIGIN); upsertLibSymbolsToY(doc, wireLibSymbols(wire), ORIGIN); + // Stamp the seed marker like the file path does: a seeded-then-emptied + // sheet must stay distinguishable from a hollow one (ydocIsHollow). + doc.getMap(Y_KDOC_META).set( + Y_KDOC_SEED_NONCE, + `${doc.clientID}:${Math.random().toString(36).slice(2)}`, + ); }, ORIGIN); return; } diff --git a/web/standalone/src/wasm/collab/sheet-manager.test.ts b/web/standalone/src/wasm/collab/sheet-manager.test.ts index 45fc1bf..d6a7087 100644 --- a/web/standalone/src/wasm/collab/sheet-manager.test.ts +++ b/web/standalone/src/wasm/collab/sheet-manager.test.ts @@ -10,8 +10,16 @@ const { connectKicadDoc, bindKicadCollab, moduleItemsBridge } = vi.hoisted(() => vi.mock("./index", () => ({ connectKicadDoc })); vi.mock("./kicad-binding", () => ({ bindKicadCollab, moduleItemsBridge })); +const { ydocHasState, syncLayoutToY, fileToDoc } = vi.hoisted(() => ({ + ydocHasState: vi.fn(() => false), + syncLayoutToY: vi.fn(() => true), + fileToDoc: vi.fn((t: string) => ({ text: t })), +})); vi.mock("@pcbjam/shared", () => ({ collabRoomId: (s: string, p: string, d: string) => `${s}:${p}:${d}`, + ydocHasState, + syncLayoutToY, + fileToDoc, })); import { createSheetCollabManager } from "./sheet-manager"; @@ -195,6 +203,35 @@ describe("sheet-manager warm pool", () => { }); }); +describe("sheet-manager layout save-sync (hollow-room guard)", () => { + beforeEach(() => { + ydocHasState.mockReset().mockReturnValue(false); + syncLayoutToY.mockReset().mockReturnValue(true); + }); + + it("skips a warmed-but-never-entered sheet whose doc is empty (save-all before first entry)", async () => { + const m = makeManager(); + await m.connectAll(["root.kicad_sch", "Headers.kicad_sch"]); + await m.switchTo("root.kicad_sch"); + // KiCad's SaveProject writes EVERY sheet → the hook fires for Headers too. + m.syncLayoutFromSave("Headers.kicad_sch", "(kicad_sch …)"); + // Nothing written: a layout-only doc would be adopted as "remove everything" + // on the first entry into Headers. + expect(syncLayoutToY).not.toHaveBeenCalled(); + }); + + it("still syncs the bound (seeded) sheet and any sheet whose doc already has state", async () => { + const m = makeManager(); + await m.connectAll(["root.kicad_sch", "Headers.kicad_sch"]); + await m.switchTo("root.kicad_sch"); + m.syncLayoutFromSave("root.kicad_sch", "(kicad_sch …)"); + expect(syncLayoutToY).toHaveBeenCalledTimes(1); + ydocHasState.mockReturnValue(true); // Headers was seeded by a peer + m.syncLayoutFromSave("Headers.kicad_sch", "(kicad_sch …)"); + expect(syncLayoutToY).toHaveBeenCalledTimes(2); + }); +}); + describe("sheet-manager lifecycle hardening (findings C-1/C-4/C-5)", () => { it("a connect resolving after destroy() is torn down, not registered (C-1)", async () => { let release!: () => void; diff --git a/web/standalone/src/wasm/collab/sheet-manager.ts b/web/standalone/src/wasm/collab/sheet-manager.ts index 27d6581..13f98cc 100644 --- a/web/standalone/src/wasm/collab/sheet-manager.ts +++ b/web/standalone/src/wasm/collab/sheet-manager.ts @@ -3,6 +3,7 @@ import { collabRoomId, fileToDoc, syncLayoutToY, + ydocHasState, type KicadDoc, type PresenceUser, } from "@pcbjam/shared"; @@ -372,6 +373,15 @@ export function createSheetCollabManager(opts: SheetManagerOptions): SheetCollab const room = rooms.get(sheetPath); if (!room) return; // not a collab sheet (or still onboarding) — nothing to sync const write = (): void => { + // A room nobody has entered yet (never bound this session) and whose + // doc is still empty has nothing to reconcile: a layout-only write + // would leave a HOLLOW doc (layout, zero items) that every later first + // entry would adopt by removing the whole sheet. The first bind + // file-seeds it — layout included — so nothing is lost by skipping. + if (!room.seeded && !ydocHasState(room.doc)) { + clog(`[sheet] layout save-sync: ${sheetPath} skipped (room never seeded)`); + return; + } try { // Writing to a PARKED room's doc marks it dirty via startWatch — fine: // the diff-on-rebind adopt makes the catch-up cost the real delta only. diff --git a/web/standalone/src/wasm/collab/sibling-restage.test.ts b/web/standalone/src/wasm/collab/sibling-restage.test.ts index 4d5c462..ae77912 100644 --- a/web/standalone/src/wasm/collab/sibling-restage.test.ts +++ b/web/standalone/src/wasm/collab/sibling-restage.test.ts @@ -13,6 +13,7 @@ vi.mock("../kicad-runner", () => ({ restageFile })); vi.mock("@pcbjam/shared", () => ({ collabRoomId: (s: string, p: string, d: string) => `${s}:${p}:${d}`, ydocHasState, + ydocIsHollow: () => false, yToDoc: (doc: unknown) => doc, docToFile: () => "(kicad_sch materialized)", })); diff --git a/web/standalone/src/wasm/collab/sibling-restage.ts b/web/standalone/src/wasm/collab/sibling-restage.ts index a8261dc..2979e50 100644 --- a/web/standalone/src/wasm/collab/sibling-restage.ts +++ b/web/standalone/src/wasm/collab/sibling-restage.ts @@ -1,4 +1,4 @@ -import { collabRoomId, docToFile, ydocHasState, yToDoc } from "@pcbjam/shared"; +import { collabRoomId, docToFile, ydocHasState, ydocIsHollow, yToDoc } from "@pcbjam/shared"; import type * as Y from "yjs"; import { restageFile } from "../kicad-runner"; import { connectKicadDoc, type KicadDocSession } from "./index"; @@ -102,6 +102,9 @@ export async function startSiblingRestage(opts: { // An empty room means no one ever seeded this sheet — the boot-staged // API snapshot is the freshest copy there is; leave it alone. if (!ydocHasState(doc)) return; + // A hollow doc (layout only, never seeded) would restage a title-block-only + // file over the real one — the staged copy is the freshest there is. + if (ydocIsHollow(doc)) return; const text = docToFile(yToDoc(doc)); restageFile(win, slug, sheetPath, new TextEncoder().encode(text), log); } catch (err) { diff --git a/wxwidgets b/wxwidgets index 4752f1b..12a699b 160000 --- a/wxwidgets +++ b/wxwidgets @@ -1 +1 @@ -Subproject commit 4752f1bfbebfd25e7a2bf376a0ffd28bbd5ac876 +Subproject commit 12a699b33590f78a9c830bbc62af584dee5382ed