diff --git a/packages/diagram-client/src/diagram-client.css b/packages/diagram-client/src/diagram-client.css index 811f072..617c5c8 100644 --- a/packages/diagram-client/src/diagram-client.css +++ b/packages/diagram-client/src/diagram-client.css @@ -880,55 +880,59 @@ i.cal-toggle-feedback-edges.cal-feedback-hidden-icon { Boundary node styles (network input/output ports) ============================================ */ -.boundary-node .boundary-body { - stroke-width: 1.5px; -} - -.boundary-node .boundary-label { - fill: var(--vscode-editor-foreground, #ffffff); +/* A boundary port is a symbol on the wire, not a box: an arrow glyph on the + axis, with the name on that same line and the type below it. All three are + drawn by the node's own view — there is no separate label element to style, + which is why nothing here reaches for `.boundary-label` any more. Sizes come + from BoundaryPortGeometry, which the server reads too, so the glyph and the + port anchor land on the same pixel. */ + +.boundary-node .boundary-name { font-family: var(--vscode-font-family); - font-size: 11px; + font-size: 12px; font-weight: 500; } -.boundary-node .boundary-type-label { - font-size: 9px; - font-weight: 400; - opacity: 0.85; -} - -.boundary-input .boundary-body { +.boundary-input .boundary-glyph, +.boundary-input .boundary-name { fill: var(--vscode-charts-green, #89d185); - fill-opacity: 0.3; - stroke: var(--vscode-charts-green, #89d185); } -.boundary-input .boundary-label { - fill: var(--vscode-charts-green, #89d185); +.boundary-output .boundary-glyph, +.boundary-output .boundary-name { + fill: var(--vscode-charts-blue, #007fd4); } -.boundary-output .boundary-body { - fill: var(--vscode-charts-blue, #007fd4); - fill-opacity: 0.3; - stroke: var(--vscode-charts-blue, #007fd4); +/* The type deliberately carries the NODES' footer class and nothing else, so a + port's type reads exactly like an entity's rather than inventing a second + convention. Tinting it by direction would also make it compete with the name + sitting right above it. */ +.boundary-node .boundary-type { + font-size: 10px; } -.boundary-output .boundary-label { - fill: var(--vscode-charts-blue, #007fd4); +/* The grab target. Invisible until the port is selected or hovered, which is + also what replaced the old body as the thing that shows selection. */ +.boundary-node .boundary-hit { + fill: transparent; + stroke: none; } -.boundary-node.selected .boundary-body { - stroke: var(--vscode-focusBorder, #007fd4); - stroke-width: 2px; +.boundary-node.hover .boundary-hit { + fill: var(--vscode-toolbar-hoverBackground, rgba(128, 128, 128, 0.18)); } -.boundary-node.hover .boundary-body { - fill-opacity: 0.5; +.boundary-node.selected .boundary-hit { + fill: none; + stroke: var(--vscode-focusBorder, #007fd4); + stroke-width: 1px; + rx: 2px; } -.boundary-node .direction-arrow { - fill: var(--vscode-foreground, #cccccc); - opacity: 0.6; +/* The port element stays in the model as the edge anchor, but the symbol above + is what is drawn — rendering both would double the arrow. */ +.boundary-node .workflow-port { + display: none; } /* ============================================ diff --git a/packages/diagram-client/src/views.ts b/packages/diagram-client/src/views.ts index 1a58e00..acabeff 100644 --- a/packages/diagram-client/src/views.ts +++ b/packages/diagram-client/src/views.ts @@ -42,7 +42,7 @@ import { import { EDGE_OPEN_ANCHOR_X_ARG, EDGE_OPEN_ANCHOR_Y_ARG } from './edge-open-mouse-listener'; import { clientBehavior } from './profile'; import { isIncidentEdgeInActiveDrag } from './elk-live-drag-router'; -import { WorkflowDiagramCss, WorkflowDiagramMetadata, WorkflowDiagramTypes } from '@dialogram/shared'; +import { BoundaryPortGeometry, WorkflowDiagramCss, WorkflowDiagramMetadata, WorkflowDiagramTypes } from '@dialogram/shared'; import { WorkflowDiagramConstants } from '@dialogram/shared'; /** Walk to the model root; the drag-active window is tracked per root. */ @@ -88,7 +88,7 @@ const NODE_HEIGHT = 80; const HEADER_HEIGHT = WorkflowDiagramConstants.HEADER_HEIGHT_PX; const PORT_RADIUS = 5; const BOUNDARY_NODE_WIDTH = 80; -const BOUNDARY_NODE_HEIGHT = 30; +const BOUNDARY_NODE_HEIGHT = BoundaryPortGeometry.ROW_PITCH_PX; // Icon size for center node icons const NODE_ICON_SIZE = 36; @@ -1017,6 +1017,123 @@ export class ProxyNodeView extends ShapeView { * Boundary input port node (left margin) - network's input exposed to internal entities * Styled as a terminal connector shape with port name centered */ +/** Whether an edge terminates on a boundary port, whose glyph is already an arrow. */ +function endsAtBoundaryPort(edge: Readonly): boolean { + let element: any = (edge as any).target; + while (element) { + if (element.type === WorkflowDiagramTypes.NODE_BOUNDARY_INPUT + || element.type === WorkflowDiagramTypes.NODE_BOUNDARY_OUTPUT) { + return true; + } + element = element.parent; + } + return false; +} + +/** One of a boundary node's own text values, from the args the server set. */ +function boundaryText(node: unknown, key: string): string { + const value = (node as { args?: Record })?.args?.[key]; + return typeof value === 'string' ? value : ''; +} + +/** + * A boundary port, drawn on the wire's axis: glyph, name and type as one object. + * + * An input is a bare arrow pointing into the diagram; an output is an arrow + * that stops against a bar, the way a terminal is drawn on a schematic. + * + * The hit target is deliberately far larger than the arrow, and deliberately + * only as tall as one row: an 8px arrow cannot be grabbed reliably, and + * including the type line would move the drag target whenever the type is + * hidden at low zoom. It also carries selection now that there is no body to + * outline. + * + * The x it is built at must agree with where the SERVER placed the port, or the + * wire misses the arrow — both read `BoundaryPortGeometry` for exactly that + * reason. + */ +function boundaryPort(isInput: boolean, nodeWidth: number, name: string, type: string): VNode[] { + const G = BoundaryPortGeometry; + const axis = G.AXIS_Y_PX; + const arrow = isInput ? G.SOURCE_ARROW : G.SINK_ARROW; + // Inputs occupy the node's right edge, outputs its left. + const glyphX = isInput ? nodeWidth - G.glyphWidth(true) : 0; + const top = axis - arrow.height / 2; + const tipX = glyphX + arrow.width; + const textX = isInput ? nodeWidth - G.textOffset(true) : G.textOffset(false); + const anchor = isInput ? 'end' : 'start'; + + // How far the text reaches, from the glyph outward. Approximate by design — + // see approximateTextWidth; it sizes the target and nothing else. + const textReach = Math.max( + G.approximateTextWidth(name, G.NAME_FONT_PX), + G.approximateTextWidth(type, G.TYPE_FONT_PX) + ); + const hit = isInput + ? { x: textX - textReach, width: textReach + G.textOffset(true) } + : { x: 0, width: G.textOffset(false) + textReach }; + + const parts: VNode[] = [ + // The whole row is the grab target, so the name belongs to the port + // rather than sitting beside it — see HIT_HEIGHT_PX. First in the list + // so it paints behind the glyph and the text. + // + // Sized to the text rather than to the node box. The box is a fixed 120 + // so that the glyph columns line up, which has nothing to do with how + // wide any particular port reads: using it made a short name leave dead + // clickable space beside it, while a long one overflowed the box and + // stopped being clickable at exactly the point it became visible. + svg('rect', { + class: { 'boundary-hit': true }, + attrs: { x: hit.x, y: 0, width: hit.width, height: G.HIT_HEIGHT_PX } + }), + // Both arrows point the way the data flows: into the diagram for an + // input, into the bar for an output. + svg('path', { + class: { 'boundary-glyph': true }, + attrs: { d: `M ${glyphX} ${top} L ${tipX} ${axis} L ${glyphX} ${top + arrow.height} Z` } + }) + ]; + if (!isInput) { + parts.push(svg('rect', { + class: { 'boundary-glyph': true, 'boundary-glyph-bar': true }, + attrs: { + x: tipX, + y: axis - G.SINK_BAR.height / 2, + width: G.SINK_BAR.width, + height: G.SINK_BAR.height + } + })); + } + + // The name belongs to the symbol, not beside it. It used to be a child + // label element positioned by its own view, which is what made it drift off + // the glyph — an element with no layout feature still carries a stale + // position, and anything that reads it moves the text. Drawn here it cannot + // disagree with the arrow it labels. + parts.push(svg('text', { + class: { 'boundary-name': true }, + // As an inline style, NOT only as an attribute. A presentation attribute + // loses to any stylesheet rule, and upstream GLSP/Sprotty styles set + // `text-anchor: middle` — which is why the name kept rendering centred + // on its own glyph however the attribute was set. The port labels below + // hit this years ago and say so; this is the same fix. + style: { 'text-anchor': anchor }, + attrs: { x: textX, y: axis, 'text-anchor': anchor, 'dominant-baseline': 'middle' } + }, name)); + + // The type reuses the nodes' own footer treatment, so a port's type reads + // exactly like an entity's does rather than inventing a second convention. + if (type) { + parts.push(svg('text', { + class: { 'type-footer-label': true, 'boundary-type': true }, + style: { 'text-anchor': anchor }, + attrs: { x: textX, y: G.TYPE_Y_PX, 'text-anchor': anchor, 'dominant-baseline': 'middle' } + }, type)); + } + return parts; +} + @injectable() export class BoundaryInputNodeView extends ShapeView { override render(node: Readonly, context: RenderingContext, args?: IViewArgs): VNode | undefined { @@ -1025,8 +1142,7 @@ export class BoundaryInputNodeView extends ShapeView { } // Use server-provided size, not Sprotty-computed bounds - const { width, height } = getNodeSize(node, BOUNDARY_NODE_WIDTH, BOUNDARY_NODE_HEIGHT); - const cornerRadius = height / 2; + const { width } = getNodeSize(node, BOUNDARY_NODE_WIDTH, BOUNDARY_NODE_HEIGHT); return svg('g', { class: { @@ -1039,17 +1155,12 @@ export class BoundaryInputNodeView extends ShapeView { transform: `translate(${node.position.x}, ${node.position.y})` } }, - // Rounded terminal shape (left side fully rounded, right side has small radius) - svg('rect', { - class: { 'boundary-body': true }, - attrs: { - x: 0, y: 0, - width, height, - rx: cornerRadius, - ry: cornerRadius - } - }), - // Children: boundary labels + output port on right side + // No body: the symbol on the axis IS the port — glyph, name and + // type together, drawn as one thing. + ...boundaryPort(true, width, boundaryText(node, WorkflowDiagramMetadata.PORT_NAME), + boundaryText(node, WorkflowDiagramMetadata.PORT_TYPE)), + // The port element (an anchor only) and the label elements, which + // render nothing — see WorkflowLabelView. ...context.renderChildren(node) ); } @@ -1067,8 +1178,7 @@ export class BoundaryOutputNodeView extends ShapeView { } // Use server-provided size, not Sprotty-computed bounds - const { width, height } = getNodeSize(node, BOUNDARY_NODE_WIDTH, BOUNDARY_NODE_HEIGHT); - const cornerRadius = height / 2; + const { width } = getNodeSize(node, BOUNDARY_NODE_WIDTH, BOUNDARY_NODE_HEIGHT); return svg('g', { class: { @@ -1081,17 +1191,9 @@ export class BoundaryOutputNodeView extends ShapeView { transform: `translate(${node.position.x}, ${node.position.y})` } }, - // Rounded terminal shape (right side fully rounded) - svg('rect', { - class: { 'boundary-body': true }, - attrs: { - x: 0, y: 0, - width, height, - rx: cornerRadius, - ry: cornerRadius - } - }), - // Children: boundary labels + input port on left side + // No body — see the input view above. + ...boundaryPort(false, width, boundaryText(node, WorkflowDiagramMetadata.PORT_NAME), + boundaryText(node, WorkflowDiagramMetadata.PORT_TYPE)), ...context.renderChildren(node) ); } @@ -1660,7 +1762,7 @@ export class WorkflowEdgeView extends PolylineEdgeView { }, svg('tspan', {}, toIndexLabel))] : []), // Arrow marker at target (use trimmed segments so arrow is at end of visible line) - this.renderArrow(normalizedSegments) + this.renderArrow(normalizedSegments, edge) ); } @@ -2039,7 +2141,16 @@ export class WorkflowEdgeView extends PolylineEdgeView { return path; } - protected renderArrow(segments: { x: number; y: number }[]): VNode { + protected renderArrow(segments: { x: number; y: number }[], edge?: Readonly): VNode { + // A boundary port is drawn AS an arrow, so an edge ending at one must + // not bring its own. Two arrowheads meeting is not the only problem: + // the tip is deliberately nudged past the endpoint to meet the stroke + // cap, and the endpoint is the glyph's own edge — so it landed inside + // the glyph. The wire is a plain line into the symbol, which is how the + // drawing shows it. + if (edge && endsAtBoundaryPort(edge)) { + return svg('g', {}); + } if (segments.length < 2) { return svg('g', {}); } @@ -2087,7 +2198,7 @@ export class WorkflowEdgeView extends PolylineEdgeView { */ @injectable() export class WorkflowNoArrowEdgeView extends WorkflowEdgeView { - protected override renderArrow(_segments: { x: number; y: number }[]): VNode { + protected override renderArrow(_segments: { x: number; y: number }[], _edge?: Readonly): VNode { return svg('g', {}); } } @@ -2131,26 +2242,20 @@ export class WorkflowLabelView implements IView { const labelType = (label as any).type as string || ''; const pos = label.position ?? { x: 0, y: 0 }; - // Boundary labels (name + type) are positioned relative to the boundary node. + // Boundary labels render NOTHING here. + // + // The boundary node view draws its own name and type as part of the port + // symbol, so drawing them again from the label elements would double the + // text. The elements stay in the model because they are what a rename + // addresses (`_label_name`), and because the server owns them — + // they simply have no appearance of their own. + // + // This is also what put the name on top of its glyph. A label positioned + // by its own view still carries whatever position it was left with, and + // the view applied it; the text and the arrow it belonged to were being + // placed by two different pieces of code that had no way to agree. if (labelType === WorkflowDiagramTypes.LABEL_BOUNDARY_NAME || labelType === WorkflowDiagramTypes.LABEL_BOUNDARY_TYPE) { - const parent: any = (label as any).parent; - const parentW = parent?.size?.width ?? parent?.bounds?.width ?? 100; - const parentH = parent?.size?.height ?? parent?.bounds?.height ?? 40; - const isType = labelType === WorkflowDiagramTypes.LABEL_BOUNDARY_TYPE; - - return svg('g', { - attrs: { transform: `translate(${pos.x}, ${pos.y})` } - }, - svg('text', { - class: { 'boundary-label': true, 'boundary-type-label': isType }, - attrs: { - x: parentW / 2, - y: isType ? (parentH / 2 + 10) : (parentH / 2 - 2), - 'text-anchor': 'middle', - 'dominant-baseline': 'middle' - } - }, label.text || '') - ); + return undefined; } // Port labels - ELK positions the label box, we handle text anchoring diff --git a/packages/diagram-client/test/boundary-port-symbol.test.ts b/packages/diagram-client/test/boundary-port-symbol.test.ts new file mode 100644 index 0000000..0525a12 --- /dev/null +++ b/packages/diagram-client/test/boundary-port-symbol.test.ts @@ -0,0 +1,166 @@ +/** + * A boundary port is ONE object, drawn by the node's own view. + * + * These assert the properties that kept failing while it was assembled from + * separate pieces: that the whole row is the object, that the text anchors the + * way it is told to, and that the type looks like a node's type. + * + * On the type in particular: it must look like a node's, and must not be tinted. + * + * This shipped wrong once. The type label carried both `boundary-label` and + * `boundary-type-label`, so `.boundary-input .boundary-label` — the rule that + * tints a NAME green — matched it too, at identical specificity. Document order + * decided, the direction rules came later, and the type rendered in the name's + * colour on both sides. Note that a colour assertion would not have caught it: + * the grey declaration was present and correct the whole time, and what was + * wrong was whether it applied. + * + * Both halves are now structural rather than a specificity fight. The type is + * drawn by the node's own view carrying the NODES' footer class, so it inherits + * one definition of what a type looks like instead of maintaining a second, and + * no selector that tints a name can reach it. + */ +import { readFileSync } from 'node:fs'; +import * as path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; +import { WorkflowDiagramTypes } from '@dialogram/shared'; +import { WorkflowEdgeView } from '../src/views'; + +const here = path.dirname(fileURLToPath(import.meta.url)); +const read = (file: string): string => readFileSync(path.resolve(here, file), 'utf8'); + +/** Comments stripped: they discuss the very selectors they explain, and an + * earlier version of this test matched a selector inside its own comment. */ +const CSS = read('../src/diagram-client.css').replace(/\/\*[\s\S]*?\*\//g, ''); +const VIEWS = read('../src/views.ts'); + +describe('boundary port symbol', () => { + it('reuses the nodes’ own footer class rather than a second convention', () => { + // The class the entity node views put on their type footer. + expect(VIEWS).toMatch(/'type-footer-label':\s*true,\s*'boundary-type':\s*true/); + }); + + it('has that class actually define the shared look', () => { + const footer = /\.type-footer-label\s*\{([^}]*)\}/.exec(CSS); + + expect(footer, '.type-footer-label no longer exists').not.toBeNull(); + expect(footer![1]).toContain('descriptionForeground'); + expect(footer![1]).toContain('italic'); + }); + + /** + * The bug, stated as a rule: whatever tints by direction may reach the name + * and the glyph, and nothing else. The type is not in that set. + */ + it('is out of reach of every direction tint', () => { + const tinted = [...CSS.matchAll(/\.boundary-(?:input|output)\s+\.([\w-]+)/g)].map(m => m[1]); + + expect(tinted.length, 'the direction rules moved or vanished').toBeGreaterThan(0); + expect([...new Set(tinted)].sort()).toEqual(['boundary-glyph', 'boundary-name']); + }); + + /** And the name still IS tinted — otherwise the rule above passes trivially. */ + it('leaves the name tinted by direction', () => { + expect(CSS).toMatch(/\.boundary-input\s+\.boundary-name\s*\{[^}]*charts-green/s); + expect(CSS).toMatch(/\.boundary-output\s+\.boundary-name\s*\{[^}]*charts-blue/s); + }); + + /** + * Anchoring has to be an inline STYLE, not only an attribute. + * + * A presentation attribute loses to any stylesheet rule, and upstream + * GLSP/Sprotty styles set `text-anchor: middle`. So the name rendered + * centred on its own glyph no matter what the attribute said — the text and + * the arrow appeared on top of each other, twice, before this was found. + * The port labels in the same file already carried the fix and a comment + * explaining it. + * + * Asserting the style specifically is the point: an assertion that merely + * found `text-anchor` somewhere would have passed throughout the bug. + */ + it('forces the text anchor as a style, which CSS cannot override', () => { + const symbol = /function boundaryPort[\s\S]*?\n\}/.exec(VIEWS); + expect(symbol, 'boundaryPort has been renamed').not.toBeNull(); + + // One per text element it draws: the name and the type. + const styled = [...symbol![0].matchAll(/style:\s*\{\s*'text-anchor':/g)]; + expect(styled.length).toBe(2); + }); + + /** + * The grab target is the node's whole row, which is what makes the name part + * of the port instead of something sitting beside it. + * + * It began as a small box centred on the glyph — reasonable, since a 10px + * arrow is hard to hit, but it meant a port could not be clicked or dragged + * by its own name, and selecting one outlined the arrow while leaving the + * name outside the outline. Asserting the WIDTH comes from the node is the + * point: any fixed number would be a box around the arrow again. + */ + it('makes the whole row the grab target, not a box around the arrow', () => { + const symbol = /function boundaryPort[\s\S]*?\n\}/.exec(VIEWS); + const hit = /'boundary-hit':\s*true[\s\S]*?attrs:\s*\{([^}]*)\}/.exec(symbol![0]); + + expect(hit, 'the hit rectangle has gone').not.toBeNull(); + // Derived from the text's reach, not from a constant and not from the + // node box — the box is a fixed width so the glyph columns line up, + // which says nothing about how wide any one port reads. + expect(hit![1]).toMatch(/x:\s*hit\.x/); + expect(hit![1]).toMatch(/width:\s*hit\.width/); + expect(symbol![0]).toMatch(/approximateTextWidth/); + }); + + /** + * A boundary port IS an arrow, so an edge ending at one must not draw + * another. They do not merely duplicate: the edge's tip is nudged past its + * endpoint to meet the stroke cap, and that endpoint is the glyph's own + * edge, so the arrowhead came to rest inside the glyph. + * + * Called for real rather than grepped. The first version of this test + * searched the source for the guard, and passed happily when the guard was + * disabled with `false &&` — it was checking that the words were present, + * not that they did anything. + */ + describe('an edge ending at a boundary port', () => { + const view = new WorkflowEdgeView(); + const segments = [{ x: 0, y: 0 }, { x: 10, y: 0 }]; + const arrowFor = (target: unknown): { tag: string } => + (view as unknown as { + renderArrow(s: typeof segments, e: unknown): { tag: string }; + }).renderArrow(segments, { target } as never); + + it('arrives as a plain line', () => { + const arrow = arrowFor({ type: WorkflowDiagramTypes.NODE_BOUNDARY_OUTPUT }); + + expect(arrow.tag).toBe('g'); + }); + + it('arrives as a plain line when it ends on the port inside the node', () => { + // Edges attach to the GPort, not the node, so the check has to walk up. + const arrow = arrowFor({ + type: 'port:output', + parent: { type: WorkflowDiagramTypes.NODE_BOUNDARY_INPUT } + }); + + expect(arrow.tag).toBe('g'); + }); + + /** The control: an ordinary edge must still get its arrowhead. */ + it('still draws one into an ordinary node', () => { + const arrow = arrowFor({ type: 'node:task', parent: undefined }); + + expect(arrow.tag).toBe('polygon'); + }); + }); + + /** + * The name and type are drawn by the node view now, so the label elements + * must draw nothing — rendering both would double every string on screen. + */ + it('renders nothing from the boundary label elements', () => { + expect(VIEWS).toMatch( + /LABEL_BOUNDARY_NAME\s*\|\|[^)]*LABEL_BOUNDARY_TYPE\)\s*\{\s*return undefined;/ + ); + }); +}); diff --git a/packages/diagram-server/src/model/graph-gmodel-source.ts b/packages/diagram-server/src/model/graph-gmodel-source.ts index c1e4ad8..f09d92a 100644 --- a/packages/diagram-server/src/model/graph-gmodel-source.ts +++ b/packages/diagram-server/src/model/graph-gmodel-source.ts @@ -1,4 +1,4 @@ -import { WorkflowDiagramCss, WorkflowDiagramConstants, WorkflowDiagramMetadata, WorkflowDiagramTypes } from '@dialogram/shared'; +import { BoundaryPortGeometry, WorkflowDiagramCss, WorkflowDiagramConstants, WorkflowDiagramMetadata, WorkflowDiagramTypes } from '@dialogram/shared'; import * as path from 'node:path'; import { findFeedbackEdges } from './feedback-edges'; import { URI } from 'vscode-uri'; @@ -1030,14 +1030,32 @@ export class GraphGModelSource { private createBoundaryNode(node: PyGraphNode, stableNodeId: string): GNode { const port = node.ports[0]; const isInput = node.kind === 'wf-input'; - const size = { width: 120, height: 50 }; + // One row tall now, not a 50px box — the port is a symbol on the wire's + // axis rather than a container. Width stays fixed across every boundary + // node so their inner edges, and therefore their glyphs, line up in a + // column whatever ELK does with alignment inside the layer; the text + // runs outward from that edge into the margin, where overflow is + // harmless. See BoundaryPortGeometry. + const size = { width: 120, height: BoundaryPortGeometry.ROW_PITCH_PX }; + const glyph = { + width: BoundaryPortGeometry.glyphWidth(isInput), + height: BoundaryPortGeometry.SOURCE_ARROW.height + }; const portId = `${stableNodeId}_port_${port?.name ?? 'Port'}`; const p: GPort = { id: portId, type: isInput ? WorkflowDiagramTypes.PORT_OUTPUT : WorkflowDiagramTypes.PORT_INPUT, cssClasses: [WorkflowDiagramCss.PORT], - position: { x: isInput ? size.width : -8, y: 22 }, - size: { width: 8, height: 6 }, + // Sits exactly where the client draws the glyph, so the anchor the + // routers compute lands on the arrow's tip: an input's arrow ends at + // the node's right edge, an output's begins at its left edge. The y + // puts the anchor on the axis — the NAME's line, not the node's + // centre, which is what used to run the wire between the two lines. + position: { + x: isInput ? size.width - glyph.width : 0, + y: BoundaryPortGeometry.AXIS_Y_PX - glyph.height / 2 + }, + size: glyph, args: { [WorkflowDiagramMetadata.PORT_NAME]: port?.name ?? '', [WorkflowDiagramMetadata.PORT_DIRECTION]: isInput ? 'output' : 'input' diff --git a/packages/diagram-server/test/boundary-port-geometry.test.ts b/packages/diagram-server/test/boundary-port-geometry.test.ts new file mode 100644 index 0000000..074cf44 --- /dev/null +++ b/packages/diagram-server/test/boundary-port-geometry.test.ts @@ -0,0 +1,123 @@ +/** + * The wire has to touch the arrow. + * + * A boundary port is drawn as a glyph on the wire's axis, and the two halves of + * that are computed in different processes: the SERVER sizes the node and places + * the port element the routers anchor to, the CLIENT draws the arrow. Nothing + * connects them at runtime — if they disagree the wire simply ends somewhere + * near the arrow instead of on it, which no test would notice and no error would + * report. + * + * So both read `BoundaryPortGeometry`, and this pins the server's half of that + * agreement: the anchor the routers compute must land exactly where the client + * puts the arrow's tip. The client's half is held by the compiler — it builds + * the glyph from the same constants. + * + * The old layout is what makes the axis worth asserting: a 50px box with the + * wire at its vertical centre put the wire in the GAP between the name and the + * type, running through the middle of the label. The axis is now the name's own + * line, and the type sits below it, clear of the wire. + */ +import { describe, expect, it } from 'vitest'; +import { BoundaryPortGeometry, portAnchor, WorkflowDiagramMetadata } from '@dialogram/shared'; +import { GraphGModelSource, type PyGraphDocument } from '../src/model/graph-gmodel-source'; + +function docWith(kind: 'wf-input' | 'wf-output'): PyGraphDocument { + return { + version: '1', + graph: { + id: 'root', + nodes: [{ + id: `wf:${kind}:Com`, + kind, + label: 'Com', + scope: 'root', + ports: [{ + id: 'p', + name: 'Com', + direction: kind === 'wf-input' ? 'out' : 'in', + type: 'Commit' + }] + }], + edges: [] + } + }; +} + +/** The boundary node and its port, as the server hands them to the client. */ +function boundaryOf(kind: 'wf-input' | 'wf-output') { + const result = new GraphGModelSource().transform(docWith(kind)); + const node: any = result.graph.children?.find((c: any) => c.args?.['wf:boundaryKind'] !== undefined); + const port: any = node.children?.find((c: any) => c.type?.startsWith('port')); + expect(port, 'boundary node has no port').toBeDefined(); + // The node sits at the origin until ELK places it, so port positions are + // already absolute for the purposes of the anchor arithmetic. + return { + node, + anchor: portAnchor({ absolute: port.position, size: port.size, type: port.type }) + }; +} + +describe('boundary port geometry', () => { + it('is one row tall, not a box', () => { + expect(boundaryOf('wf-input').node.size.height).toBe(BoundaryPortGeometry.ROW_PITCH_PX); + }); + + it('anchors the wire on the name line, not the node centre', () => { + const input = boundaryOf('wf-input'); + const output = boundaryOf('wf-output'); + + expect(input.anchor.y).toBe(BoundaryPortGeometry.AXIS_Y_PX); + expect(output.anchor.y).toBe(BoundaryPortGeometry.AXIS_Y_PX); + // The distinction the redesign turns on: the old anchor was here. + expect(input.anchor.y).not.toBe(input.node.size.height / 2); + }); + + it("anchors an input at its arrow's tip, on the node's right edge", () => { + const { node, anchor } = boundaryOf('wf-input'); + + expect(anchor.side).toBe('EAST'); + expect(anchor.x).toBe(node.size.width); + }); + + it("anchors an output where its arrow begins, on the node's left edge", () => { + const { anchor } = boundaryOf('wf-output'); + + expect(anchor.side).toBe('WEST'); + expect(anchor.x).toBe(0); + }); + + it('keeps the type off the wire', () => { + const G = BoundaryPortGeometry; + // The name owns the axis; the type's line starts below the name's. + expect(G.AXIS_Y_PX).toBeLessThan(G.NAME_LINE_HEIGHT_PX); + expect(G.TYPE_Y_PX).toBeGreaterThan(G.NAME_LINE_HEIGHT_PX); + expect(G.NAME_LINE_HEIGHT_PX + G.TYPE_LINE_HEIGHT_PX).toBe(G.ROW_PITCH_PX); + }); + + it('leaves a gap between glyph and text on both sides', () => { + expect(BoundaryPortGeometry.textOffset(true)) + .toBe(BoundaryPortGeometry.SOURCE_ARROW.width + BoundaryPortGeometry.GLYPH_TEXT_GAP_PX); + // An output's glyph is arrow plus bar, so its text starts further out. + expect(BoundaryPortGeometry.textOffset(false)) + .toBe(BoundaryPortGeometry.SINK_ARROW.width + BoundaryPortGeometry.SINK_BAR.width + + BoundaryPortGeometry.GLYPH_TEXT_GAP_PX); + }); + + /** + * The grab target is the row, which is what makes the name part of the port + * rather than something sitting next to it. Its height is the fixed pitch + * and not anything measured from the text, so hiding the type line at low + * zoom cannot move the target. + */ + it('keeps the grab target a fixed row tall', () => { + expect(BoundaryPortGeometry.HIT_HEIGHT_PX).toBe(BoundaryPortGeometry.ROW_PITCH_PX); + }); + + it('still carries the port identity', () => { + const { node } = boundaryOf('wf-input'); + + expect(node.args[WorkflowDiagramMetadata.PORT_NAME]).toBe('Com'); + expect(node.args[WorkflowDiagramMetadata.PORT_TYPE]).toBe('Commit'); + }); +}); diff --git a/packages/shared/src/boundary-port-geometry.ts b/packages/shared/src/boundary-port-geometry.ts new file mode 100644 index 0000000..a592949 --- /dev/null +++ b/packages/shared/src/boundary-port-geometry.ts @@ -0,0 +1,112 @@ +/** + * The geometry of a boundary port: a schematic port symbol, not a box. + * + * A network's inputs and outputs used to draw as rounded pills with the name + * centred and the type under it. Two lines of text inside a 50px box, the wire + * meeting it at the box's vertical centre — which is the gap BETWEEN the two + * lines, so the wire ran through the middle of the label. + * + * This is the schematic treatment instead: an arrow glyph sitting on the wire's + * own axis, with the text running outward from it, away from the wire. The name + * sits ON the axis so the wire enters on the name line; the type sits on a + * second line below, clear of the wire entirely. Names are right-aligned on + * inputs and left-aligned on outputs, so both text columns run away from the + * glyph column and the glyphs stay in a straight line. + * + * It lives in shared because the two halves of it are computed in different + * processes and must agree exactly: the SERVER sizes the node and places the + * port (which is what the edge routers anchor to), the CLIENT draws the glyph + * and the two text lines. If the client's glyph and the server's port anchor + * disagree, the wire visibly misses the arrow it is supposed to touch. + */ + +/** Arrow, bar, or hit-target extents. */ +export interface BoundaryGlyphBox { + width: number; + height: number; +} + +export namespace BoundaryPortGeometry { + // The drawing these came from is set at a finer weight than the diagram + // actually renders at: against a 3px wire, a 9.5px name and an 8px arrow + // read as fine print beside a cable. Everything below is that drawing scaled + // by about a quarter, keeping its proportions — the arrow still spans most + // of the name's line, the type still sits a little under two thirds of the + // name's size, and the gap still reads as one space. + + /** Total height of one port: the name line plus the type line. */ + export const ROW_PITCH_PX = 28; + /** The name's line box. Its centre is the axis. */ + export const NAME_LINE_HEIGHT_PX = 15; + /** The type's line box, directly below the name's. */ + export const TYPE_LINE_HEIGHT_PX = 13; + + export const NAME_FONT_PX = 12; + export const TYPE_FONT_PX = 10; + + /** Between the glyph and the first character of text. */ + export const GLYPH_TEXT_GAP_PX = 5; + + /** An input's glyph: a plain arrow, pointing into the diagram. */ + export const SOURCE_ARROW: BoundaryGlyphBox = { width: 10, height: 12 }; + /** An output's glyph: an arrow that stops against a bar, like a terminal. */ + export const SINK_ARROW: BoundaryGlyphBox = { width: 9, height: 12 }; + export const SINK_BAR: BoundaryGlyphBox = { width: 2.5, height: 14 }; + + /** + * The grab target: the node's whole row, not a box around the arrow. + * + * It started as a small rectangle centred on the glyph, on the reasoning + * that a 10px arrow is too small to hit reliably. True, but it made the name + * something that merely sat NEXT to the port rather than part of it — you + * could not click or drag a port by its own name, and selecting one outlined + * the arrow while leaving the name outside. A port is one object; its whole + * row is that object. + * + * The width is the node's, so it is the caller that supplies it. The height + * stays the fixed row pitch rather than anything measured from the text, + * which is what keeps the original property: hiding the type line at low + * zoom moves nothing, because the target never depended on it. + */ + export const HIT_HEIGHT_PX = ROW_PITCH_PX; + + /** Where the wire meets the port, measured down from the node's top. */ + export const AXIS_Y_PX = NAME_LINE_HEIGHT_PX / 2; + + /** Centre of the type line, measured down from the node's top. */ + export const TYPE_Y_PX = NAME_LINE_HEIGHT_PX + TYPE_LINE_HEIGHT_PX / 2; + + /** + * A rough width for a run of text, used ONLY to size the grab target. + * + * The real width is a browser measurement the model has no access to, and + * asking for one would mean a render pass. An estimate is fine here because + * nothing about the drawing depends on it: the glyph, the text and the port + * anchor are all positioned from fixed geometry, and this decides only how + * far the clickable area reaches. Being a few pixels out makes the target + * slightly generous or slightly tight, and nothing moves either way. + * + * It must never be used to lay anything out. + */ + export function approximateTextWidth(text: string, fontPx: number): number { + // Averaged across the mixed-case identifiers these labels actually hold; + // the UI font is proportional, so no single ratio is right for all of + // them, and erring wide costs nothing but a slightly larger target. + return text.length * fontPx * 0.6; + } + + /** Full width of a glyph, including an output's bar. */ + export function glyphWidth(isInput: boolean): number { + return isInput ? SOURCE_ARROW.width : SINK_ARROW.width + SINK_BAR.width; + } + + /** + * Where text starts, as an offset from the node's own inner edge. + * + * Inputs read right-to-left from the node's right edge; outputs left-to-right + * from its left edge. Either way the text begins one gap past the glyph. + */ + export function textOffset(isInput: boolean): number { + return glyphWidth(isInput) + GLYPH_TEXT_GAP_PX; + } +} diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 5d4202e..13390c9 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -9,6 +9,7 @@ export * from './diagram-types'; export * from './diagram-constants'; export * from './port-anchor'; export * from './port-stub'; +export * from './boundary-port-geometry'; export * from './binding-keys'; export * from './diagram-seams'; export * from './chat-seams';