From cbc74d840974a1e537b08e483811dec5194f2d53 Mon Sep 17 00:00:00 2001 From: David Montero Crespo Date: Mon, 3 Aug 2026 23:04:37 +0200 Subject: [PATCH] fix(ui): the inspector dropped labels for inset pin columns, and flickered Three things the dialog got wrong in use: 1. Missing pins. Classification was nearest-edge with an "interior" bucket, and interior pins printed no label - only a dot with a tooltip. The ReSpeaker Lite showed it: its XIAO socket is two INSET columns (x=50.5 and 126.5 on a 178-wide body) plus two breakout columns, so most of its pins were called interior and the part looked like it was missing them. The classifier now finds the LINES first - pins sharing an x are a column, pins sharing a y a row - assigns each line to the side of the body it sits on however far inset it is, and joins label to dot with a leader. Only a lone pad falls back to nearest-edge. Every pin gets a printed label; that is now the contract, covered by a ReSpeaker test with its real table. 2. The panel was cut off on the Datasheet tab until some unrelated event re-rendered it. The datasheet loads asynchronously, so the one-shot measure placed the dialog while it was still short and it then grew off screen. A ResizeObserver repositions on any size change, and the tab body caps its own height so long markdown scrolls inside the tab. 3. Flicker when the pointer left and re-entered. The preview effect depended on componentProperties and pinInfo by identity - both are rebuilt on every parent render (pinInfo is a getter) - so the element was torn down and remounted constantly. It now depends on stable digests of them. --- .../__tests__/pin-inspector-layout.test.ts | 58 ++++++--- .../simulator/PartInspectorDialog.css | 13 +- .../simulator/PartInspectorDialog.tsx | 92 +++++++++----- frontend/src/utils/pinInspectorLayout.ts | 113 +++++++++++++----- 4 files changed, 202 insertions(+), 74 deletions(-) diff --git a/frontend/src/__tests__/pin-inspector-layout.test.ts b/frontend/src/__tests__/pin-inspector-layout.test.ts index e0a93663..7a26064d 100644 --- a/frontend/src/__tests__/pin-inspector-layout.test.ts +++ b/frontend/src/__tests__/pin-inspector-layout.test.ts @@ -148,20 +148,50 @@ describe('layoutInspectorPins — a crowded header fits its edge', () => { }); }); -describe('layoutInspectorPins — interior pins', () => { - it('detects a pin far from every edge and anchors its label on the dot', () => { - const r = layoutInspectorPins( - [ - { name: 'MID', x: 50, y: 50 }, - { name: 'L', x: 2, y: 50 }, - ], - { width: 100, height: 100 }, - ); - const mid = r.pins.find((p) => p.name === 'MID')!; - expect(mid.edge).toBe('interior'); - expect(mid.labelX).toBe(mid.dotX); - expect(mid.labelY).toBe(mid.dotY); - expect(r.pins.find((p) => p.name === 'L')!.edge).toBe('left'); +describe('layoutInspectorPins — inset columns (the ReSpeaker Lite case)', () => { + // pro/.../SeeedElements.ts ReSpeakerLiteElement.pinInfo, body 178x437. + // The XIAO socket is TWO INSET COLUMNS (x=50.5 / 126.5 on a 178-wide body) + // plus two breakout pad columns at x=10. A nearest-edge rule called most of + // these "interior" and printed no label, so the part looked like it was + // missing pins. Every pin must get a label. + const RESPEAKER: InspectorPinInput[] = [ + ...['D0', 'D1', 'D2', 'D3', 'D4', 'D5', 'D6'].map((name, i) => ({ + name, + x: 50.5, + y: [43, 55, 68, 81, 93, 106, 119][i], + })), + ...['D7', 'D8', 'D9', 'D10', '3V3', 'GND', '5V'].map((name, i) => ({ + name, + x: 126.5, + y: [43, 55, 68, 81, 93, 106, 119][i], + })), + ...['BTN1', 'BTN2', 'BTN3'].map((name, i) => ({ name, x: 10, y: 214 + i * 15 })), + ...['IO1', 'IO2', 'IO3', 'IO4'].map((name, i) => ({ name, x: 10, y: 289 + i * 13 })), + ]; + + const r = layoutInspectorPins(RESPEAKER, { width: 178, height: 437 }); + + it('labels every single pin — none silently dropped', () => { + expect(r.pins).toHaveLength(RESPEAKER.length); + const named = new Set(r.pins.map((p) => p.name)); + for (const p of RESPEAKER) expect(named.has(p.name)).toBe(true); + }); + + it('keeps each inset column on the side of the body it sits on', () => { + const sideOf = (n: string) => r.pins.find((p) => p.name === n)!.edge; + for (const n of ['D0', 'D3', 'D6']) expect(sideOf(n)).toBe('left'); + for (const n of ['D7', '3V3', '5V']) expect(sideOf(n)).toBe('right'); + for (const n of ['BTN1', 'IO4']) expect(sideOf(n)).toBe('left'); + }); + + it('draws a leader from every inset dot to its gutter label', () => { + for (const n of ['D0', 'D7', '3V3']) { + expect(r.pins.find((p) => p.name === n)!.needsLeader).toBe(true); + } + }); + + it('never puts a column end-pin in the top or bottom gutter', () => { + expect(r.pins.some((p) => p.edge === 'top' || p.edge === 'bottom')).toBe(false); }); }); diff --git a/frontend/src/components/simulator/PartInspectorDialog.css b/frontend/src/components/simulator/PartInspectorDialog.css index 5192767c..f45a5988 100644 --- a/frontend/src/components/simulator/PartInspectorDialog.css +++ b/frontend/src/components/simulator/PartInspectorDialog.css @@ -168,6 +168,15 @@ display: block; } +/* Stand-in for the part's body while the art is hidden (pin mode). */ +.pid-body-outline { + position: absolute; + border: 1px dashed #5a5a5a; + border-radius: 4px; + background: rgba(255, 255, 255, 0.02); + pointer-events: none; +} + .pid-preview-missing { color: #666; font-size: 22px; @@ -352,7 +361,9 @@ overscroll-behavior: contain; -webkit-overflow-scrolling: touch; min-height: 120px; - max-height: min(52vh, 420px); + /* Cap the tab, not the dialog: a long datasheet scrolls INSIDE the tab so + the dialog keeps a stable height and never runs off the screen. */ + max-height: min(46vh, 360px); } /* The datasheet reuses the picker card's .cip-doc styles as-is. */ diff --git a/frontend/src/components/simulator/PartInspectorDialog.tsx b/frontend/src/components/simulator/PartInspectorDialog.tsx index bb20bb96..c45b877c 100644 --- a/frontend/src/components/simulator/PartInspectorDialog.tsx +++ b/frontend/src/components/simulator/PartInspectorDialog.tsx @@ -76,8 +76,14 @@ interface PartInspectorDialogProps { wireInProgress?: boolean; } -const PREVIEW_ART = 150; // collapsed preview box -const PINS_ART = 240; // art box when the pin view is on +const PREVIEW_ART = 150; // collapsed preview box (the part's own art) +/** + * Pin-map box. Smaller than the collapsed preview on purpose: in this mode the + * part's art is HIDDEN (it ate the dialog for no information) and only the pin + * dots, their labels and a faint outline of the body are drawn, so the spatial + * arrangement still reads while the panel stays compact. + */ +const PINS_ART = 190; export const PartInspectorDialog: React.FC = ({ componentId, @@ -147,6 +153,17 @@ export const PartInspectorDialog: React.FC = ({ return () => window.removeEventListener('keydown', onCaptureKey, true); }, [capturingKey, componentId, onPropertyChange]); + // The preview effect must NOT re-run on every parent render, or the element + // is torn down and remounted and the dialog visibly flickers whenever the + // canvas re-renders (moving the pointer in and out is enough). Both of the + // inputs it cares about arrive as fresh objects each render — pinInfo is a + // getter that builds a new array, and componentProperties a new object — so + // depend on stable string digests of them instead of their identities. + const propsKey = Object.entries(componentProperties) + .map(([k, v]) => `${k}=${typeof v === 'string' && v.length > 32 ? v.length : String(v)}`) + .join('|'); + const pinsKey = pinInfo.map((p) => `${p.name}:${p.x},${p.y}`).join('|'); + // ── Live preview: mount the real element, measure, lay out pins ────────── useEffect(() => { const host = previewHostRef.current; @@ -218,26 +235,45 @@ export const PartInspectorDialog: React.FC = ({ el.removeEventListener('pininfo-change', onPinsChange); host.textContent = ''; }; - // componentProperties is intentionally in the deps: a color change while - // the dialog is open should repaint the preview. - }, [componentMetadata, componentProperties, pinInfo, showPins]); + // propsKey/pinsKey stand in for componentProperties/pinInfo: a real change + // (a colour edit, an LED flip) changes the digest and repaints; a bare + // re-render does not. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [componentMetadata, propsKey, pinsKey, showPins]); // ── Position: viewport coords, clamped, portaled to body ───────────────── + // Re-run on every SIZE change, not just on the inputs we can name. The + // datasheet arrives asynchronously (loadDoc) and the markdown can be long, + // so a one-shot measure placed the dialog while it was still short and it + // then grew off the bottom of the screen — the user only saw it corrected + // when some other event happened to re-render and re-measure it. useLayoutEffect(() => { const el = dialogRef.current; if (!el) return; - const margin = 12; - const w = el.offsetWidth; - const h = el.offsetHeight; - const vw = window.innerWidth; - const vh = window.innerHeight; - let x = position.x + 16; - let y = position.y; - if (x + w > vw - margin) x = Math.max(margin, position.x - w - 16); - x = Math.max(margin, Math.min(x, vw - w - margin)); - y = Math.max(margin, Math.min(y, vh - h - margin)); - setDialogPos({ x, y }); - }, [position, layout, tab, showPins]); + const place = () => { + const margin = 12; + const w = el.offsetWidth; + const h = el.offsetHeight; + const vw = window.innerWidth; + const vh = window.innerHeight; + let x = position.x + 16; + let y = position.y; + if (x + w > vw - margin) x = Math.max(margin, position.x - w - 16); + x = Math.max(margin, Math.min(x, vw - w - margin)); + y = Math.max(margin, Math.min(y, vh - h - margin)); + setDialogPos((prev) => + prev && Math.abs(prev.x - x) < 0.5 && Math.abs(prev.y - y) < 0.5 ? prev : { x, y }, + ); + }; + place(); + const ro = new ResizeObserver(place); + ro.observe(el); + window.addEventListener('resize', place); + return () => { + ro.disconnect(); + window.removeEventListener('resize', place); + }; + }, [position]); // ── Close behaviours (unchanged semantics) ─────────────────────────────── useEffect(() => { @@ -376,18 +412,16 @@ export const PartInspectorDialog: React.FC = ({ onClick={clickable ? activate : undefined} disabled={!clickable} /> - {p.edge !== 'interior' && ( - - )} + ); })} diff --git a/frontend/src/utils/pinInspectorLayout.ts b/frontend/src/utils/pinInspectorLayout.ts index af8877f6..495d61d5 100644 --- a/frontend/src/utils/pinInspectorLayout.ts +++ b/frontend/src/utils/pinInspectorLayout.ts @@ -151,25 +151,83 @@ export function layoutInspectorPins( const W = w * scale; const H = h * scale; - // Classify each pin by its nearest edge on the scaled art. - const fW = interiorFrac * W; - const fH = interiorFrac * H; - const classified = pins.map((pin) => { - const x = pin.x * scale; - const y = pin.y * scale; - const dLeft = x; - const dRight = W - x; - const dTop = y; - const dBottom = H - y; - let edge: PinEdge; - if (dLeft > fW && dRight > fW && dTop > fH && dBottom > fH) { - edge = 'interior'; - } else { - const min = Math.min(dLeft, dRight, dTop, dBottom); - edge = min === dLeft ? 'left' : min === dRight ? 'right' : min === dTop ? 'top' : 'bottom'; + // Classify each pin. + // + // Nearest-edge alone is wrong for real parts, and got caught by the + // ReSpeaker Lite: its XIAO socket is two INSET columns (x=50.5 and 126.5 on + // a 178-wide body) plus two rows of breakout pads. Every one of those sat + // far enough from all four edges to be called "interior", which printed no + // label — the pins were on screen as dots and looked missing. And a plain + // nearest-edge rule scatters a column's end pins to the top/bottom gutter. + // + // So: find the LINES first. Pins sharing an x form a vertical column, pins + // sharing a y form a horizontal row; a column belongs to the side of the + // body it sits on, however far inset it is, and a leader line joins its + // labels to the dots. Only a pin that belongs to no line falls back to its + // nearest edge. Every pin gets a printed label — that is the contract. + const TOL = 6 * scale; // pads within this are the same column/row + const cluster = (vals: number[]): number[][] => { + const idx = vals.map((v, i) => [v, i] as const).sort((a, b) => a[0] - b[0]); + const groups: number[][] = []; + let cur: number[] = []; + let last = NaN; + for (const [v, i] of idx) { + if (cur.length && Math.abs(v - last) > TOL) { + groups.push(cur); + cur = []; + } + cur.push(i); + last = v; + } + if (cur.length) groups.push(cur); + return groups; + }; + + const xs = pins.map((p) => p.x * scale); + const ys = pins.map((p) => p.y * scale); + const side = new Array(pins.length).fill(null); + + // Vertical columns win first: they are the dominant shape on breakout + // boards, shields and every DIP part. + for (const g of cluster(xs)) { + if (g.length < 2) continue; + const spanY = Math.max(...g.map((i) => ys[i])) - Math.min(...g.map((i) => ys[i])); + if (spanY < TOL) continue; // that is a row, not a column + const cx = g.reduce((s, i) => s + xs[i], 0) / g.length; + const e: PinEdge = cx <= W / 2 ? 'left' : 'right'; + for (const i of g) side[i] = e; + } + // Then horizontal rows, for whatever is still unassigned (headers). + for (const g of cluster(ys)) { + const free = g.filter((i) => side[i] === null); + if (free.length < 2) continue; + const spanX = Math.max(...free.map((i) => xs[i])) - Math.min(...free.map((i) => xs[i])); + if (spanX < TOL) continue; + const cy = free.reduce((s, i) => s + ys[i], 0) / free.length; + const e: PinEdge = cy <= H / 2 ? 'top' : 'bottom'; + for (const i of free) side[i] = e; + } + + const classified = pins.map((pin, i) => { + const x = xs[i]; + const y = ys[i]; + let edge = side[i]; + if (!edge) { + // A lone pad: nearest edge. + const d = [ + ['left', x], + ['right', W - x], + ['top', y], + ['bottom', H - y], + ] as Array<[PinEdge, number]>; + d.sort((a, b) => a[1] - b[1]); + edge = d[0][0]; } return { pin, x, y, edge }; }); + // interiorFrac is retained in the options for callers that want the old + // behaviour; the line-first classifier makes it unnecessary here. + void interiorFrac; // Lay labels per edge. Left/right stack along y; top/bottom along x // (with wider spacing — their labels are horizontal text). @@ -217,20 +275,15 @@ export function layoutInspectorPins( }; }); } - for (const [c, i] of byEdge('interior')) { - // Interior pins keep their label ON the dot: the dialog renders a - // tooltip instead of printed text (a label in the middle of the art - // would sit on top of the silkscreen). - laid[i] = { - name: c.pin.name, - edge: 'interior', - dotX: c.x, - dotY: c.y, - labelX: c.x, - labelY: c.y, - needsLeader: false, - signalKind: signalKindOf(c.pin), - }; + // A leader is also needed whenever the DOT is inset from its gutter, even + // if the label sits on the dot's own row: without it an inset column's + // labels look unattached to anything. + for (const p of laid) { + if (!p) continue; + if (p.edge === 'left' && p.dotX > 6) p.needsLeader = true; + else if (p.edge === 'right' && p.dotX < W - 6) p.needsLeader = true; + else if (p.edge === 'top' && p.dotY > 6) p.needsLeader = true; + else if (p.edge === 'bottom' && p.dotY < H - 6) p.needsLeader = true; } const has = (e: PinEdge) => classified.some((c) => c.edge === e);