From 1797fef5372673dff5093987b05bba88616c7a85 Mon Sep 17 00:00:00 2001 From: Endri Bezati Date: Thu, 5 Nov 2026 12:31:14 +0100 Subject: [PATCH 1/4] feat: libavoid draws the routes after a drop, edges on one port as one path; a dragged node's edges wait for the drop The routes an edge cannot keep after a move (see the slide) are now drawn by libavoid, with every edge on a port started at one point so that libavoid routes them as one shared path: a trunk with a junction, as the layout draws a fan-in. The staggered start the wrapper used gave every edge on a port its own channel, which is why libavoid drew the same comb as the built-in router and was left opt-in. The half-nudge kink the stagger was for is taken out after routing instead: a step smaller than the nudging distance at a port goes back onto the port's row. libavoid is the default router now; the built-in one is the fallback when libavoid is unavailable or cannot solve an edge, the choice for a reroute of every edge in a diagram of more than 200 edges, and available throughout with WORKFLOW_DIAGRAM_ROUTER=manhattan. The vendored build is found from the sources too, so a dev run and the tests route with libavoid without an environment variable. On the client, the edges of a node being dragged are not drawn while it moves. Before, they were drawn on their stored routes with a stub trailing the pointer, then jumped at the drop. They come back with the model update that follows the drop's reroute, or after 1.5s if none comes, or at once when a drag is cancelled or the reroute fails. A move that is finished as it arrives (an arrow key) hides nothing. Verified on the gemmini state document: a 60px and a 400px move of "The loop configuration" keep ELK's trunk, and the one straight edge libavoid routes joins the port cleanly; on the rob:Com fixture the two edges off one port share a stub and part at a junction, where the first had an 8px kink before. Claude-Session: https://claude.ai/code/session_01EToj6RJRMSSHQiqGNMU9wv --- CHANGELOG.md | 17 +++ .../src/elk-live-drag-router.ts | 84 ++++++++++- .../src/stock-features.module.ts | 6 +- packages/diagram-client/src/views.ts | 7 +- .../test/container-parity.test.ts | 3 +- .../test/edges-hidden-while-dragged.test.ts | 101 +++++++++++++ .../fixtures/container-parity.baseline.json | 10 ++ packages/diagram-client/test/glsp-stub.ts | 2 + .../reroute-edges-avoid-overlaps-handler.ts | 30 ++-- .../src/routing/libavoid-router.ts | 58 +++++++- .../test/libavoid-router.test.ts | 134 +++++++++--------- .../reroute-keeps-the-layouts-trunk.test.ts | 18 ++- 12 files changed, 371 insertions(+), 99 deletions(-) create mode 100644 packages/diagram-client/test/edges-hidden-while-dragged.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 81e8097..5afc5b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,23 @@ platform API version. ### Changed +- **After a drop, the routes an edge cannot keep are drawn by libavoid, with + the edges on one port as one shared path.** The layout draws the edges into + a port as a trunk; the built-in router gives every edge its own channel and + drew a comb, and libavoid did the same because every edge on a port was + started at its own staggered point. Now they start at one point and + libavoid keeps them on one path for as long as they go the same way, a + trunk with a junction. The half-nudge kink the stagger was for is taken + out after routing. libavoid is the router from now on; the built-in one + remains the fallback when libavoid is unavailable or cannot solve an edge, + the choice for a reroute of every edge in a diagram of more than 200 + edges, and available throughout with `WORKFLOW_DIAGRAM_ROUTER=manhattan`. +- **The edges of a node being dragged are not drawn while it moves.** They + come back with the drop's routes, as a schematic editor redraws a block's + nets after it is placed; before, they trailed the pointer on their old + routes and jumped at the drop. A move by arrow key hides nothing. If the + routes do not arrive within 1.5s the edges are drawn as they were. + - **The Java ELK server is rebuilt on ELK 0.10.0, and elkjs is pinned to 0.10.0**, so a graph lays out the same on either engine; before, the server's ELK 0.8.1 put some graphs' nodes in another order than elkjs 0.9.3. diff --git a/packages/diagram-client/src/elk-live-drag-router.ts b/packages/diagram-client/src/elk-live-drag-router.ts index 7ab0113..e219b75 100644 --- a/packages/diagram-client/src/elk-live-drag-router.ts +++ b/packages/diagram-client/src/elk-live-drag-router.ts @@ -1,6 +1,6 @@ import { EditorContextService, IActionDispatcher, TYPES } from '@eclipse-glsp/client'; import type { GModelElement, SEdgeImpl } from '@eclipse-glsp/client'; -import { Action, IActionHandler, MoveAction } from '@eclipse-glsp/sprotty'; +import { Action, IActionHandler, MoveAction, SetModelAction, UpdateModelAction } from '@eclipse-glsp/sprotty'; import { inject, injectable } from 'inversify'; import { WorkflowDiagramTypes } from '@dialogram/shared'; import { WorkflowRerouteEdgesAvoidOverlapsOperation } from './editing-action-handlers'; @@ -90,6 +90,25 @@ function setIncidentEdgesForDrag(root: Readonly, edgeIds: Set>(); + +/** Whether this edge is hidden because the node it is attached to is being dragged. */ +export function isEdgeHiddenForDrag(root: Readonly | undefined, edgeId: string): boolean { + if (!root) { + return false; + } + return dragHiddenEdgeIds.get(getDragKey(root))?.has(edgeId) ?? false; +} + +/** How long hidden edges may wait for the server's routes before they are drawn as they were. */ +const REVEAL_FALLBACK_MS = 1500; + @injectable() export class WorkflowElkLiveDragRouter implements IActionHandler { @inject(EditorContextService) @@ -117,7 +136,19 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { /** Which path armed the pending reroute — reported to the server for timing. */ private pendingTrigger: 'finished' | 'quiet' | 'requeue' = 'quiet'; + /** The moved ids the hidden edge set was computed for, so a drag computes it once. */ + private hiddenForMovedIds = ''; + /** Set when the final reroute went out: the next model update carries its routes. */ + private revealOnNextModelUpdate = false; + private revealTimer: ReturnType | undefined; + handle(action: Action): void { + if (action.kind === UpdateModelAction.KIND || action.kind === SetModelAction.KIND) { + if (this.revealOnNextModelUpdate) { + this.revealHiddenEdges(); + } + return; + } if (action.kind !== MoveAction.KIND) { return; } @@ -155,6 +186,14 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { // treat brief pauses mid-drag as a drag end. markAnyDragActive(root, 650); + // A drag in progress hides the moved nodes' edges until the drop's + // routes arrive. A move that is finished as it arrives (an arrow key) + // hides nothing: the routes follow within the round trip, and a blink + // on every key press would be worse than the stub it replaces. + if ((action as any).finished !== true) { + this.hideIncidentEdges(root); + } + // Schedule a final reroute shortly after the MoveAction stream goes quiet. if (this.dragEndTimer) { clearTimeout(this.dragEndTimer); @@ -186,6 +225,42 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { // Preview rerouting is disabled; compute incident edges once on drag-end/quiet. } + private hideIncidentEdges(root: Readonly): void { + const movedIds = Array.from(this.pendingMovedElementIds).sort().join('\u0000'); + if (movedIds === this.hiddenForMovedIds && dragHiddenEdgeIds.has(getDragKey(root))) { + return; + } + const { incidentEdgeIds } = this.collectIncidentEdgesFromMovedElements(root, this.pendingMovedElementIds, new Map()); + this.hiddenForMovedIds = movedIds; + if (incidentEdgeIds.size === 0) { + return; + } + dragHiddenEdgeIds.set(getDragKey(root), incidentEdgeIds); + this.revealOnNextModelUpdate = false; + } + + /** The hidden edges wait for the server's routes from now on, or for the fallback. */ + private armReveal(): void { + if (dragHiddenEdgeIds.size === 0) { + return; + } + this.revealOnNextModelUpdate = true; + if (this.revealTimer) { + clearTimeout(this.revealTimer); + } + this.revealTimer = setTimeout(() => this.revealHiddenEdges(), REVEAL_FALLBACK_MS); + } + + private revealHiddenEdges(): void { + if (this.revealTimer) { + clearTimeout(this.revealTimer); + this.revealTimer = undefined; + } + this.revealOnNextModelUpdate = false; + this.hiddenForMovedIds = ''; + dragHiddenEdgeIds.clear(); + } + private collectIncidentEdgesFromMovedElements( root: Readonly, movedElementIds: Set, @@ -348,6 +423,8 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { if (!preview) { this.forceNonPreviewFlush = false; this.lastDispatchedEdgeIds = []; + // Nothing to reroute (a cancelled drag): draw the edges as they are. + this.revealHiddenEdges(); } return; } @@ -365,6 +442,9 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { } this.dispatchInFlight = true; + if (!effectivePreview) { + this.armReveal(); + } try { await this.actionDispatcher.dispatch( WorkflowRerouteEdgesAvoidOverlapsOperation.create({ @@ -387,6 +467,8 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { this.lastDispatchedEdgeIds = []; } } catch (e) { + // The routes are not coming: draw the edges as they are. + this.revealHiddenEdges(); // Re-queue so we can retry on the next tick; avoids getting stuck with stale routes. for (const id of edgeIds) { this.dispatchPendingEdgeIds.add(id); diff --git a/packages/diagram-client/src/stock-features.module.ts b/packages/diagram-client/src/stock-features.module.ts index ea5df0d..06afadf 100644 --- a/packages/diagram-client/src/stock-features.module.ts +++ b/packages/diagram-client/src/stock-features.module.ts @@ -11,7 +11,7 @@ */ import { ContainerModule } from 'inversify'; import { ChangeBoundsTool, TYPES, bindAsService, configureActionHandler } from '@eclipse-glsp/client'; -import { MoveAction } from '@eclipse-glsp/sprotty'; +import { MoveAction, SetModelAction, UpdateModelAction } from '@eclipse-glsp/sprotty'; import { PropertyPanel } from './property-panel'; import { ChatPanel } from './chat-panel-integrated'; import { clientBehavior } from './profile'; @@ -152,7 +152,11 @@ export const workflowFeaturesModule = new ContainerModule((bind, unbind, isBound rebind(ChangeBoundsTool).to(WorkflowChangeBoundsTool).inSingletonScope(); // Drag-time rerouting: client streams MoveAction updates; server recomputes routes (ELK fixed). + // The moved node's edges are hidden through the drag and drawn again with + // the model update that carries the drop's routes. configureActionHandler(context, MoveAction.KIND, WorkflowElkLiveDragRouter); + configureActionHandler(context, UpdateModelAction.KIND, WorkflowElkLiveDragRouter); + configureActionHandler(context, SetModelAction.KIND, WorkflowElkLiveDragRouter); // The live tier: edges routed in the webview during a drag, with the same // router and the same anchors the server uses on commit, so the committed diff --git a/packages/diagram-client/src/views.ts b/packages/diagram-client/src/views.ts index fb30f2e..654ec9d 100644 --- a/packages/diagram-client/src/views.ts +++ b/packages/diagram-client/src/views.ts @@ -44,7 +44,7 @@ import { import { EDGE_OPEN_ANCHOR_X_ARG, EDGE_OPEN_ANCHOR_Y_ARG } from './edge-open-mouse-listener'; import { clientBehavior } from './profile'; import { captionIsShared, carriedTextOf } from './edge-caption-sharing'; -import { isIncidentEdgeInActiveDrag } from './elk-live-drag-router'; +import { isEdgeHiddenForDrag, isIncidentEdgeInActiveDrag } from './elk-live-drag-router'; import { BoundaryPortGeometry, WorkflowDiagramCss, WorkflowDiagramMetadata, WorkflowDiagramTypes, normalizePortSide } from '@dialogram/shared'; import { WorkflowDiagramConstants } from '@dialogram/shared'; @@ -1765,6 +1765,11 @@ export class WorkflowEdgeView extends PolylineEdgeView { // is not visible. That shared geometry is what made this possible: the // client router previously computed anchors differently, which is why // this code used to bypass the router registry entirely. + // An edge of a node being dragged is not drawn until the drop's routes + // arrive (see `isEdgeHiddenForDrag`). + if (isEdgeHiddenForDrag(rootOfEdge(edge), String((edge as any).id))) { + return svg('g', { class: { 'workflow-edge': true, 'edge-hidden-for-drag': true } }); + } const serverRoute = (edge as any).routingPoints as { x: number; y: number }[] | undefined; const hasServerRoute = Array.isArray(serverRoute) && serverRoute.length >= 2; const liveRouted = isIncidentEdgeInActiveDrag(rootOfEdge(edge), String((edge as any).id)); diff --git a/packages/diagram-client/test/container-parity.test.ts b/packages/diagram-client/test/container-parity.test.ts index a087eb5..df8cc79 100644 --- a/packages/diagram-client/test/container-parity.test.ts +++ b/packages/diagram-client/test/container-parity.test.ts @@ -32,7 +32,8 @@ * states, and the read-only view marker (hides the palette's tools while the view shown is * read-only, bound to itself and as a model-root listener), and the selection trace (its listener, * bound to itself and as a selection listener, and the `SelectionTraceFeedbackCommand` that draws - * the path) — plus one change: `label:boundary:type` is bound to + * the path), and the drag router as the `updateModel` and `setModel` handler (it draws a dragged + * node's edges again when the drop's routes arrive) — plus one change: `label:boundary:type` is bound to * the non-editable `BoundaryLabel`, because editing it renamed the port. * The oracle still does its job: it fails on any binding this composition gains * or loses, and updating the fixture is the deliberate act of accepting one. diff --git a/packages/diagram-client/test/edges-hidden-while-dragged.test.ts b/packages/diagram-client/test/edges-hidden-while-dragged.test.ts new file mode 100644 index 0000000..35dfebd --- /dev/null +++ b/packages/diagram-client/test/edges-hidden-while-dragged.test.ts @@ -0,0 +1,101 @@ +/** + * The edges of a node being dragged are not drawn while it moves: they come + * back with the model update that carries the drop's routes. A move that is + * finished as it arrives (an arrow key) hides nothing. + */ +import 'reflect-metadata'; +import { describe, expect, it, vi } from 'vitest'; +import { WorkflowElkLiveDragRouter, isEdgeHiddenForDrag } from '../src/elk-live-drag-router'; + +type Element = { id: string; type: string; parent?: Element; children?: Element[]; sourceId?: string; targetId?: string }; + +/** a →(a-t)→ t, b →(b-t)→ t, c →(c-d)→ d, each node with an in and an out port. */ +function model(): { root: any; elements: Map } { + const elements = new Map(); + const root: any = { id: 'root', type: 'graph', children: [] as Element[] }; + const node = (id: string): void => { + const n: Element = { id, type: 'node:actor', parent: root, children: [] }; + for (const port of ['in', 'out']) { + const p: Element = { id: `${id}.${port}`, type: port === 'in' ? 'port:input' : 'port:output', parent: n }; + n.children!.push(p); elements.set(p.id, p); + } + root.children.push(n); elements.set(id, n); + }; + const edge = (from: string, to: string): void => { + const e: Element = { id: `${from}-${to}`, type: 'edge:connection', parent: root, sourceId: `${from}.out`, targetId: `${to}.in` }; + root.children.push(e); elements.set(e.id, e); + }; + for (const id of ['a', 'b', 't', 'c', 'd']) { node(id); } + edge('a', 't'); edge('b', 't'); edge('c', 'd'); + root.index = { getById: (id: string) => elements.get(id), all: () => elements.values() }; + return { root, elements }; +} + +function router(root: any): { handler: WorkflowElkLiveDragRouter; dispatched: any[] } { + const dispatched: any[] = []; + const handler: any = new WorkflowElkLiveDragRouter(); + handler.editorContextService = { modelRoot: root }; + handler.actionDispatcher = { dispatch: async (action: any) => { dispatched.push(action); } }; + return { handler, dispatched }; +} + +const move = (id: string, finished = false): any => ({ kind: 'move', moves: [{ elementId: id, toPosition: { x: 10, y: 10 } }], finished }); +const hidden = (root: any, ...ids: string[]): boolean[] => ids.map(id => isEdgeHiddenForDrag(root, id)); + +describe('edges hidden while their node is dragged', () => { + it('hides the moved node\'s edges through the drag, not the others', () => { + const { root } = model(); + const { handler } = router(root); + handler.handle(move('t')); + expect(hidden(root, 'a-t', 'b-t', 'c-d')).toEqual([true, true, false]); + handler.handle(move('t')); + expect(hidden(root, 'a-t', 'b-t', 'c-d')).toEqual([true, true, false]); + handler.handle({ kind: 'updateModel' }); + handler.handle({ kind: 'updateModel' }); + handler.handle({ kind: 'setModel' }); + expect(hidden(root, 'a-t', 'b-t')).toEqual([true, true]); + }); + + it('draws them again with the model update after the drop\'s reroute went out', async () => { + vi.useFakeTimers(); + try { + const { root } = model(); + const { handler, dispatched } = router(root); + handler.handle(move('t')); + handler.handle(move('t', true)); + // The update that answers the ChangeBounds, before the reroute is sent, does not reveal. + handler.handle({ kind: 'updateModel' }); + expect(hidden(root, 'a-t')).toEqual([true]); + await vi.advanceTimersByTimeAsync(100); + expect(dispatched.length).toBe(1); + expect(hidden(root, 'a-t')).toEqual([true]); + handler.handle({ kind: 'updateModel' }); + expect(hidden(root, 'a-t', 'b-t')).toEqual([false, false]); + } finally { + vi.useRealTimers(); + } + }); + + it('draws them as they were when no routes arrive', async () => { + vi.useFakeTimers(); + try { + const { root } = model(); + const { handler } = router(root); + handler.handle(move('t')); + handler.handle(move('t', true)); + await vi.advanceTimersByTimeAsync(100); + expect(hidden(root, 'a-t')).toEqual([true]); + await vi.advanceTimersByTimeAsync(1600); + expect(hidden(root, 'a-t')).toEqual([false]); + } finally { + vi.useRealTimers(); + } + }); + + it('hides nothing for a move by arrow key', () => { + const { root } = model(); + const { handler } = router(root); + handler.handle(move('t', true)); + expect(hidden(root, 'a-t', 'b-t')).toEqual([false, false]); + }); +}); diff --git a/packages/diagram-client/test/fixtures/container-parity.baseline.json b/packages/diagram-client/test/fixtures/container-parity.baseline.json index 53c75c5..b558ca9 100644 --- a/packages/diagram-client/test/fixtures/container-parity.baseline.json +++ b/packages/diagram-client/test/fixtures/container-parity.baseline.json @@ -481,5 +481,15 @@ "id": "Symbol(stub.ISelectionListener)", "op": "bind", "to": "service:SelectionTraceListener" + }, + { + "handler": "WorkflowElkLiveDragRouter", + "kind": "updateModel", + "op": "actionHandler" + }, + { + "handler": "WorkflowElkLiveDragRouter", + "kind": "setModel", + "op": "actionHandler" } ] diff --git a/packages/diagram-client/test/glsp-stub.ts b/packages/diagram-client/test/glsp-stub.ts index 0139bc0..17a70c2 100644 --- a/packages/diagram-client/test/glsp-stub.ts +++ b/packages/diagram-client/test/glsp-stub.ts @@ -30,6 +30,8 @@ export const ApplyLabelEditOperation = { /** Drag-time reroute trigger — the base container binds a handler on this kind. */ export const MoveAction = { KIND: 'move' }; +export const UpdateModelAction = { KIND: 'updateModel' }; +export const SetModelAction = { KIND: 'setModel' }; export const NavigateToExternalTargetAction = { KIND: 'navigateToExternalTarget', diff --git a/packages/diagram-server/src/operations/reroute-edges-avoid-overlaps-handler.ts b/packages/diagram-server/src/operations/reroute-edges-avoid-overlaps-handler.ts index 13746a5..7bcde50 100644 --- a/packages/diagram-server/src/operations/reroute-edges-avoid-overlaps-handler.ts +++ b/packages/diagram-server/src/operations/reroute-edges-avoid-overlaps-handler.ts @@ -142,20 +142,25 @@ export class WorkflowRerouteEdgesAvoidOverlapsOperationHandler extends Operation private readonly debugReroute: number = Number(process.env.WORKFLOW_DIAGRAM_DEBUG_REROUTE ?? '0') || 0; /** - * Which router computes edge routes. + * Which router computes the routes an edge cannot keep (see `slideRoute`). * - * libavoid measures better than the built-in router on every axis in an - * offline harness (crossings, overlaps, bends, length), but it changed drag - * behaviour for the worse on a real network in a way the harness does not - * reproduce, so it is OPT-IN until that is understood: + * libavoid, routing the edges on one port as one shared path: that is + * what the layout drew, a trunk into the port, and what the built-in + * router cannot draw, since it gives every edge its own channel. libavoid + * also measures better on crossings, overlaps, bends and length. What + * made it worse on a real drag before was the stagger of the endpoints on + * a port, which turned every fan-in into a comb; `sharedPortPaths` is the + * cure. The built-in router remains the fallback when libavoid is + * unavailable or cannot solve an edge, and the choice for a reroute of + * every edge in a large diagram, since libavoid's cost is superlinear in + * the connector count (58 edges 337ms). * - * WORKFLOW_DIAGRAM_ROUTER=libavoid - * - * Do not flip the default back without a reproduction — see - * `dumpRoutingInput` below for capturing one. + * WORKFLOW_DIAGRAM_ROUTER=manhattan the built-in router throughout */ private readonly useLibavoid: boolean = - (process.env.WORKFLOW_DIAGRAM_ROUTER ?? '').toLowerCase() === 'libavoid'; + (process.env.WORKFLOW_DIAGRAM_ROUTER ?? '').toLowerCase() !== 'manhattan'; + /** A reroute of every edge goes to libavoid only up to this many edges. */ + private static readonly LIBAVOID_MAX_EDGES = 200; /** * `WORKFLOW_DIAGRAM_DUMP_ROUTING=1` writes the exact router input — node @@ -1190,7 +1195,7 @@ export class WorkflowRerouteEdgesAvoidOverlapsOperationHandler extends Operation if (timing) { timing.anchorMs = Date.now() - anchorStartedAt; } const routeStartedAt = Date.now(); let usedLibavoid = false; - if (this.useLibavoid) { + if (this.useLibavoid && routable.length <= WorkflowRerouteEdgesAvoidOverlapsOperationHandler.LIBAVOID_MAX_EDGES) { const shapes: RouterObstacle[] = obstacles.map(o => ({ id: o.id, // `obstacles` carries PAD already baked in; libavoid keeps its own @@ -1212,7 +1217,8 @@ export class WorkflowRerouteEdgesAvoidOverlapsOperationHandler extends Operation idealNudgingDistance: WorkflowRerouteEdgesAvoidOverlapsOperationHandler.EDGE_GAP, // Matches the built-in router's stub, so edges leave a port the // same distance either way and clearance stays at PAD. - portStub: WorkflowRerouteEdgesAvoidOverlapsOperationHandler.STUB + portStub: WorkflowRerouteEdgesAvoidOverlapsOperationHandler.STUB, + sharedPortPaths: true }); if (routes) { usedLibavoid = true; diff --git a/packages/diagram-server/src/routing/libavoid-router.ts b/packages/diagram-server/src/routing/libavoid-router.ts index 309e777..1993691 100644 --- a/packages/diagram-server/src/routing/libavoid-router.ts +++ b/packages/diagram-server/src/routing/libavoid-router.ts @@ -101,12 +101,23 @@ export interface LibavoidRoutingOptions { * and the stub is axis-aligned so the result stays orthogonal. */ portStub?: number; + /** + * Edges on one port start at one point, so libavoid routes them as one + * shared path: a trunk with a junction where they part, the way a layout + * draws a fan-in and a schematic draws a bus out of a pin. Off, each edge + * on a port starts at its own staggered point and is nudged into its own + * channel, which reads as a comb of parallel lines where edges meet. + * The shared endpoints are not nudged apart (libavoid's default), so an + * edge still leaves its port straight. + */ + sharedPortPaths?: boolean; } const DEFAULT_OPTIONS: Required = { shapeBufferDistance: 15, idealNudgingDistance: 8, - portStub: 20 + portStub: 20, + sharedPortPaths: true }; /** @@ -125,6 +136,8 @@ function candidateDirs(): string[] { if (typeof __dirname === 'string') { dirs.push(path.join(__dirname, 'libavoid')); dirs.push(__dirname); + // A dev run or a test from the sources: the vendored build in the repo. + dirs.push(path.join(__dirname, '..', '..', '..', '..', 'vendor', 'libavoid')); } return dirs; } @@ -188,6 +201,32 @@ export async function isLibavoidAvailable(): Promise { * traversable. Returns Infinity when there is nothing to constrain. */ +/** + * Put a route end that libavoid nudged off its port's row back onto it. + * + * Edges that start at one point are not nudged apart as shared paths, but the + * nudging of segments connected to shapes still moves their first segments + * half a nudging distance each way, and the L back to the anchor then draws a + * 4px kink a few pixels off the port. A step smaller than the nudging + * distance is that kink, not a turn; the first segment goes back onto the + * row, and the bend after it follows, so the edges leave the port together + * and part where their ways part. + */ +function unkink(points: RouterPoint[], anchor: RouterPoint, nudge: number): void { + if (points.length < 2) { + return; + } + const atStart = Math.abs(points[0].x - anchor.x) <= Math.abs(points[points.length - 1].x - anchor.x); + const i0 = atStart ? 0 : points.length - 1; + const i1 = atStart ? 1 : points.length - 2; + const step = Math.abs(points[i0].y - anchor.y); + if (step < 0.01 || step >= nudge || Math.abs(points[i0].y - points[i1].y) > 0.01) { + return; + } + points[i0] = { x: points[i0].x, y: anchor.y }; + points[i1] = { x: points[i1].x, y: anchor.y }; +} + /** * The corner needed to join a port anchor to the route, when libavoid's end * does not sit on the anchor's row. Empty when they already align. @@ -248,11 +287,13 @@ export async function routeOrthogonal( // invalidate, derive the ceiling from the anchors actually passed in. // Push each endpoint out along its port's side before routing; the stub back // to the true anchor is re-attached afterwards. - // Staggered where a port carries several edges, so they do not all turn on - // one x and blur together at the port. The rule is shared with the client's - // live router — see `portStubDistances` — because the two must agree or the - // edge visibly jumps the moment the mouse is released. - const stubs = portStubOffsets( + // + // With `sharedPortPaths` every edge on a port starts at the same point and + // libavoid keeps them on one path for as long as they go the same way. + // Otherwise they are staggered, so they do not all turn on one x, the rule + // the client's live router shares (`portStubOffsets`). + const base: PortStubOffset = { distance: requested.portStub, across: 0 }; + const stubs = requested.sharedPortPaths ? new Map() : portStubOffsets( connectors.flatMap(c => [ ...(c.sourceSide ? [{ id: c.id, end: 'source' as const, ...c.source, side: c.sourceSide, towardY: c.target.y }] @@ -264,7 +305,6 @@ export async function routeOrthogonal( obstacles, { portStub: requested.portStub, nudge: requested.idealNudgingDistance } ); - const base: PortStubOffset = { distance: requested.portStub, across: 0 }; const offsetOf = (p: RouterPoint, side: 'WEST' | 'EAST' | undefined, at: PortStubOffset): RouterPoint => side === 'EAST' ? { x: p.x + at.distance, y: p.y + at.across } : side === 'WEST' ? { x: p.x - at.distance, y: p.y + at.across } @@ -361,6 +401,10 @@ export async function routeOrthogonal( // So the join is an L where the lines differ: out along the // anchor's row first, then across to meet the route. Orthogonal, // and the nudging survives. + if (opts.sharedPortPaths) { + unkink(points, c.source, opts.idealNudgingDistance); + unkink(points, c.target, opts.idealNudgingDistance); + } const withStubs = [c.source, ...bridge(c.source, points[0]), ...points, ...bridge(c.target, points[points.length - 1]).reverse(), c.target]; routes.set(id, snapRouteEndpoints(withStubs, c.source, c.target)); diff --git a/packages/diagram-server/test/libavoid-router.test.ts b/packages/diagram-server/test/libavoid-router.test.ts index 6aee65d..0573ee8 100644 --- a/packages/diagram-server/test/libavoid-router.test.ts +++ b/packages/diagram-server/test/libavoid-router.test.ts @@ -103,6 +103,37 @@ describe('libavoid router', () => { expect(new Set(ys).size).toBe(4); }); + it('routes the edges on one port as one shared path, leaving the port straight', async () => { + // Two sources feeding one input port, the fan-in a layout draws as a + // trunk. Shared endpoints keep the two on one path from where they + // meet to the port; staggered endpoints made a comb of two channels. + const obstacles: RouterObstacle[] = [ + { id: 'a', x: 0, y: 0, width: 100, height: 40 }, + { id: 'b', x: 0, y: 200, width: 100, height: 40 }, + { id: 't', x: 400, y: 100, width: 100, height: 40 } + ]; + const into = (id: string, y: number): RouterConnector => ({ + id, source: { x: 100, y }, target: { x: 400, y: 120 }, sourceSide: 'EAST', targetSide: 'WEST' + }); + const connectors = [into('a-t', 20), into('b-t', 220)]; + + const shared = (await routeOrthogonal(obstacles, connectors))!; + const [ra, rb] = [shared.get('a-t')!, shared.get('b-t')!]; + // The last segment, into the port, lies on the port's row and the two + // edges arrive down the same vertical. + for (const r of [ra, rb]) { + expect(r[r.length - 1]).toEqual({ x: 400, y: 120 }); + expect(r[r.length - 2].y).toBe(120); + expect(r[0].y).toBe(r[1].y); + } + expect(ra[ra.length - 2].x).toBe(rb[rb.length - 2].x); + expect(ra[ra.length - 3].x).toBe(rb[rb.length - 3].x); + + const staggered = (await routeOrthogonal(obstacles, connectors, { sharedPortPaths: false }))!; + const [sa, sb] = [staggered.get('a-t')!, staggered.get('b-t')!]; + expect(sa[sa.length - 3].x).not.toBe(sb[sb.length - 3].x); + }); + it('returns an empty map for no connectors without touching the solver', async () => { const routes = await routeOrthogonal([{ id: 'a', x: 0, y: 0, width: 10, height: 10 }], []); expect(routes).toBeDefined(); @@ -354,88 +385,53 @@ describe('libavoid router — several edges leaving one port', () => { } }); - it('still separates two edges that share a port', async () => { + /** + * Two edges leaving one port go out together and part where their ways + * part, the way a layout draws a fan-out and a schematic draws a bus out + * of a pin: one shared stub, a junction, then each its own way. + * + * This replaces the staggered stub, which started each edge on a port at + * its own point so that libavoid would give each its own channel. That + * drew every fan-in and fan-out as a comb of parallel lines, which is the + * scramble a move of a few pixels made of the layout's trunks. The kink + * that the stagger was for (libavoid nudging two edges with one start + * half a nudging distance each way, an L back to the port 4px off its + * row) is taken out after routing instead: a step smaller than the + * nudging distance at a port goes back onto the port's row. + */ + it('takes two edges off one port on one stub, parting at a junction', async () => { const routes = (await routeOrthogonal(fx.nodes, connectors, opts))!; const shared = connectors.filter(c => c.id.includes('rob:Com')); expect(shared.length).toBe(2); + const [a, b] = shared.map(c => routes.get(c.id)!); - // They start at the same anchor, so they must diverge somewhere: the - // long horizontal runs need to sit on different rows. - const rows = shared.map(c => { - const r = routes.get(c.id)!; + // Both leave the port along its row, straight, with no step back onto it. + for (const r of [a, b]) { + expect(orthogonal(r)).toBe(true); + const firstOffRow = r.slice(1).find(p => Math.abs(p.y - r[0].y) > 0.5); + expect(Math.abs(firstOffRow!.y - r[0].y)).toBeGreaterThan(opts.idealNudgingDistance); + } + // They share the stub, at least the port stub long, then part. + const sharedUntilX = Math.min(a[1].x, b[1].x); + expect(Math.abs(sharedUntilX - a[0].x)).toBeGreaterThanOrEqual(opts.portStub); + expect(a[1].y).toBe(b[1].y); + // And they end up on different rows, each heading for its own target. + const longestRow = (r: typeof a): number => { let longest = { len: 0, y: 0 }; for (let i = 1; i < r.length; i++) { - if (Math.abs(r[i].y - r[i - 1].y) > 0.5) continue; + if (Math.abs(r[i].y - r[i - 1].y) > 0.5) { continue; } const len = Math.abs(r[i].x - r[i - 1].x); - if (len > longest.len) longest = { len, y: r[i].y }; + if (len > longest.len) { longest = { len, y: r[i].y }; } } return longest.y; - }); - expect(Math.abs(rows[0] - rows[1])).toBeGreaterThan(4); - }); - - /** - * The reported artifact, and the one that matters most: an edge on a shared - * port must leave it STRAIGHT, like an edge on any other port. - * - * Handed two edges with the same start, libavoid separates them the only way - * it can — half the nudging distance each, in opposite directions. Both then - * come back off the anchor's row, both need an L to get back to it, and a - * 4px step a few pixels off the port does not read as a branch. It reads as - * a kink in an edge that should have gone straight out, which is exactly - * what was reported at `rob.Com` (`Rb` and `Trap`, one edge each, leave - * perfectly straight right below it). - * - * So the starts are separated BEFORE routing instead: the first edge keeps - * the port's row and libavoid has no reason to move it. A "real" turn is one - * bigger than the nudging distance; anything at or below that is the kink. - */ - it('leaves the first edge on a shared port straight', async () => { - const routes = (await routeOrthogonal(fx.nodes, connectors, opts))!; - const shared = connectors - .filter(c => c.id.includes('rob:Com')) - .sort((a, b) => (a.id < b.id ? -1 : 1)); - expect(shared.length).toBe(2); - - /** How far the route first departs the anchor's row. */ - const firstTurn = (id: string): number => { - const r = routes.get(id)!; - const step = r.slice(1).find(p => Math.abs(p.y - r[0].y) > 0.5); - return step ? Math.abs(step.y - r[0].y) : Infinity; }; - - // The first edge goes straight out and only leaves the row to head for - // its target. - expect(firstTurn(shared[0].id)).toBeGreaterThan(opts.idealNudgingDistance); - // The second branches off it by a FULL nudge, not the half-nudge kink - // that libavoid produces when it has to separate them itself. - expect(firstTurn(shared[1].id)).toBeGreaterThanOrEqual(opts.idealNudgingDistance); + expect(a[a.length - 1].y).not.toBe(b[b.length - 1].y); + expect(longestRow(a) === longestRow(b) || a[a.length - 2].x !== b[b.length - 2].x).toBe(true); }); - /** - * ...and they must not TURN at the same place either, which is the third - * way this join has looked broken. - * - * Every end starts `portStub` off its port, so the L back to the anchor - * turned at exactly that distance for every edge — put two edges on one port - * and both corners landed on one x (444 here). The routes then ran - * superimposed for the whole stub and split there in a 4px step. At stroke - * width, with rounded joins, that is not a fan-out; it is a blob on the port, - * which is what was reported at `rob.Com`. - * - * libavoid separates such corners by itself when both edges turn and run a - * long way (three edges off one port in `fixtures-dense-core` come back - * 12px apart with no help). It cannot here, because the first move is only - * the 4px nudge — far too short for segment nudging to bite. Hence the - * staggered stub, and hence this test: the previous two assertions both pass - * with every corner piled on one x. - */ - it('does not turn two edges off one port at the same x', async () => { - const routes = (await routeOrthogonal(fx.nodes, connectors, opts))!; + it('staggers the starts on a port when shared paths are off', async () => { + const routes = (await routeOrthogonal(fx.nodes, connectors, { ...opts, sharedPortPaths: false }))!; const shared = connectors.filter(c => c.id.includes('rob:Com')); - expect(shared.length).toBe(2); - - // The corner is the last point still on the anchor's own row. const corners = shared.map(c => { const r = routes.get(c.id)!; let i = 1; diff --git a/packages/diagram-server/test/reroute-keeps-the-layouts-trunk.test.ts b/packages/diagram-server/test/reroute-keeps-the-layouts-trunk.test.ts index 39173d7..ba2dec8 100644 --- a/packages/diagram-server/test/reroute-keeps-the-layouts-trunk.test.ts +++ b/packages/diagram-server/test/reroute-keeps-the-layouts-trunk.test.ts @@ -4,7 +4,8 @@ * one port share a trunk; routing a moved node's edges anew, one at a time, * turned that trunk into a comb of parallel lines, and a move of a few * pixels scrambled the picture. The router runs only for an edge whose kept - * route would not do. + * route would not do, and that router is libavoid, which routes the edges + * on one port as one shared path. */ import 'reflect-metadata'; import { describe, expect, it } from 'vitest'; @@ -51,11 +52,14 @@ function graph(...elements: Array): { root: GGraph; handler: any; return { root, handler, find: id => index.find(id) }; } -async function moveAndReroute(g: ReturnType, nodeId: string, to: Point, edgeIds: string[]): Promise { +/** Moves the node, reroutes the edges, and says which router drew the routes the slide refused. */ +async function moveAndReroute(g: ReturnType, nodeId: string, to: Point, edgeIds: string[]): Promise { g.find(nodeId).position = to; + const timing = { router: 'none' }; await g.handler.executeOperation({ kind: 'dialogram.rerouteEdgesAvoidOverlaps', isOperation: true, elementIds: edgeIds, preview: false, source: 'changeBounds' - }); + }, timing); + return timing.router; } const routeOf = (g: ReturnType, id: string): Point[] => g.find(id).routingPoints; @@ -79,7 +83,7 @@ function fanIn(): ReturnType { describe('reroute after a move keeps the layout\'s routes', () => { it('slides the ends onto the ports and keeps the trunk, at any distance', async () => { const g = fanIn(); - await moveAndReroute(g, 't', { x: 400, y: 160 }, ['a-t', 'b-t']); + expect(await moveAndReroute(g, 't', { x: 400, y: 160 }, ['a-t', 'b-t'])).toBe('slide'); const movedIn = { x: 400, y: 189.5 }; expect(routeOf(g, 'a-t')).toEqual(trunk(A_OUT, movedIn)); expect(routeOf(g, 'b-t')).toEqual(trunk(B_OUT, movedIn)); @@ -106,7 +110,7 @@ describe('reroute after a move keeps the layout\'s routes', () => { edge('a-t', 'a', 't', trunk(A_OUT, T_IN)), edge('b-t', 'b', 't', trunk(B_OUT, T_IN)) ); - await moveAndReroute(g, 't', { x: 400, y: 250 }, ['a-t', 'b-t']); + expect(await moveAndReroute(g, 't', { x: 400, y: 250 }, ['a-t', 'b-t'])).toBe('libavoid'); const xBox = { id: 'x', x: 230, y: 240, r: 370, b: 300 }; for (const id of ['a-t', 'b-t']) { const route = routeOf(g, id); @@ -121,7 +125,7 @@ describe('reroute after a move keeps the layout\'s routes', () => { it('routes anew an edge whose stub would turn back through its node', async () => { const g = fanIn(); // t now stands left of the trunk: a kept route would enter it from behind. - await moveAndReroute(g, 't', { x: 200, y: 100 }, ['a-t', 'b-t']); + expect(await moveAndReroute(g, 't', { x: 200, y: 100 }, ['a-t', 'b-t'])).toBe('libavoid'); for (const id of ['a-t', 'b-t']) { const route = routeOf(g, id); expect(route[route.length - 1]).toEqual({ x: 200, y: 129.5 }); @@ -132,7 +136,7 @@ describe('reroute after a move keeps the layout\'s routes', () => { it('routes anew a straight edge, which has no bend to carry', async () => { const g = graph(node('c', 0, 0), node('d', 400, 0), edge('c-d', 'c', 'd', [{ x: 140, y: 29.5 }, { x: 400, y: 29.5 }])); - await moveAndReroute(g, 'd', { x: 400, y: 60 }, ['c-d']); + expect(await moveAndReroute(g, 'd', { x: 400, y: 60 }, ['c-d'])).toBe('libavoid'); const route = routeOf(g, 'c-d'); expect(route.length).toBeGreaterThanOrEqual(4); expect(isOrthogonalPolyline(route)).toBe(true); From 1e04ebb74d87925377ba373b65aacba6c28d2152 Mon Sep 17 00:00:00 2001 From: Endri Bezati Date: Thu, 5 Nov 2026 13:20:38 +0100 Subject: [PATCH 2/4] fix: a hidden edge is drawn again as soon as its route reaches its ports; reroutes log their time Reported: after a drop the edges took a second or more to come back. The hidden edges waited for a model update by name, the one after the final reroute, and when that was not the update that reached the drag router they stayed hidden until the 1.5s fallback. An edge is now hidden only while its stored route does not start and end at its ports as they stand: the first update with a route for the node's new place draws it, whichever server round trip brings it (the ChangeBounds answer already carries one), and a cancelled drag draws it at once, since the node goes back to where the route is. No model update handler, so the container parity baseline is as it was. The fallback stays for a route that never comes. The reroute handler logs one always-on `[dialogram perf] reroute` line per persisted reroute, like the layout's: edges kept, edges routed, the router and the time taken, so a slow drop can be read from the output channel. Claude-Session: https://claude.ai/code/session_01EToj6RJRMSSHQiqGNMU9wv --- CHANGELOG.md | 11 +- .../src/elk-live-drag-router.ts | 80 ++++++++++--- .../src/stock-features.module.ts | 8 +- packages/diagram-client/src/views.ts | 6 +- .../test/container-parity.test.ts | 3 +- .../test/edges-hidden-while-dragged.test.ts | 111 ++++++++++-------- .../fixtures/container-parity.baseline.json | 10 -- packages/diagram-client/test/glsp-stub.ts | 2 - .../reroute-edges-avoid-overlaps-handler.ts | 10 ++ 9 files changed, 152 insertions(+), 89 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5afc5b8..55e9be1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,10 +32,13 @@ platform API version. the choice for a reroute of every edge in a diagram of more than 200 edges, and available throughout with `WORKFLOW_DIAGRAM_ROUTER=manhattan`. - **The edges of a node being dragged are not drawn while it moves.** They - come back with the drop's routes, as a schematic editor redraws a block's - nets after it is placed; before, they trailed the pointer on their old - routes and jumped at the drop. A move by arrow key hides nothing. If the - routes do not arrive within 1.5s the edges are drawn as they were. + come back as soon as a route for the node's new place is in the model, as + a schematic editor redraws a block's nets after it is placed; before, they + trailed the pointer on their old routes and jumped at the drop. A move by + arrow key hides nothing, and a cancelled drag draws them again at once. If + no route arrives within 1.5s of the drop the edges are drawn as they were. +- The server logs one `[dialogram perf] reroute` line per reroute it + persists: edges kept, edges routed, the router and the time taken. - **The Java ELK server is rebuilt on ELK 0.10.0, and elkjs is pinned to 0.10.0**, so a graph lays out the same on either engine; before, the diff --git a/packages/diagram-client/src/elk-live-drag-router.ts b/packages/diagram-client/src/elk-live-drag-router.ts index e219b75..fe356aa 100644 --- a/packages/diagram-client/src/elk-live-drag-router.ts +++ b/packages/diagram-client/src/elk-live-drag-router.ts @@ -1,8 +1,8 @@ import { EditorContextService, IActionDispatcher, TYPES } from '@eclipse-glsp/client'; import type { GModelElement, SEdgeImpl } from '@eclipse-glsp/client'; -import { Action, IActionHandler, MoveAction, SetModelAction, UpdateModelAction } from '@eclipse-glsp/sprotty'; +import { Action, IActionHandler, MoveAction } from '@eclipse-glsp/sprotty'; import { inject, injectable } from 'inversify'; -import { WorkflowDiagramTypes } from '@dialogram/shared'; +import { WorkflowDiagramTypes, portAnchor } from '@dialogram/shared'; import { WorkflowRerouteEdgesAvoidOverlapsOperation } from './editing-action-handlers'; type ModelIndexLike = { @@ -98,12 +98,69 @@ function setIncidentEdgesForDrag(root: Readonly, edgeIds: Set>(); -/** Whether this edge is hidden because the node it is attached to is being dragged. */ -export function isEdgeHiddenForDrag(root: Readonly | undefined, edgeId: string): boolean { +/** + * Whether this edge is hidden because the node it is attached to is being + * dragged. It is hidden from the first move until its stored route reaches + * its ports again: at the drop, with the first model update that carries a + * route for the node's new place, whichever server round trip brings it, or + * at once when the drag is cancelled and the node goes back. No update is + * waited for by name, since the ChangeBounds answer, the reroute answer or + * a bounds pass may be the one that carries the route. + */ +export function isEdgeHiddenForDrag(root: Readonly | undefined, edge: Readonly): boolean { if (!root) { return false; } - return dragHiddenEdgeIds.get(getDragKey(root))?.has(edgeId) ?? false; + const hidden = dragHiddenEdgeIds.get(getDragKey(root)); + const id = String((edge as any).id); + if (!hidden?.has(id)) { + return false; + } + if (routeReachesPorts(root, edge)) { + hidden.delete(id); + return false; + } + return true; +} + +function absolutePosition(element: any): { x: number; y: number } { + let x = 0; + let y = 0; + for (let current = element; current; current = current.parent) { + x += current.position?.x ?? 0; + y += current.position?.y ?? 0; + } + return { x, y }; +} + +function anchorOfPort(port: any): { x: number; y: number } | undefined { + if (!port || !isPortElement(port)) { + return undefined; + } + return portAnchor({ + absolute: absolutePosition(port), + size: port.size ?? port.bounds, + direction: port.args?.['cal:portDirection'] as string | undefined, + side: port.args?.['cal:portSide'] as string | undefined, + type: port.type + }); +} + +/** Whether the edge's stored route starts and ends where its ports are now. */ +function routeReachesPorts(root: Readonly, edge: Readonly): boolean { + const route = (edge as any).routingPoints as Array<{ x: number; y: number }> | undefined; + if (!Array.isArray(route) || route.length < 2) { + return false; + } + const index: ModelIndexLike | undefined = (root as any)?.index; + const source = anchorOfPort(getById(index, (edge as any).sourceId)); + const target = anchorOfPort(getById(index, (edge as any).targetId)); + if (!source || !target) { + return false; + } + const near = (a: { x: number; y: number }, b: { x: number; y: number }): boolean => + Math.abs(a.x - b.x) < 0.5 && Math.abs(a.y - b.y) < 0.5; + return near(route[0], source) && near(route[route.length - 1], target); } /** How long hidden edges may wait for the server's routes before they are drawn as they were. */ @@ -138,17 +195,9 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { /** The moved ids the hidden edge set was computed for, so a drag computes it once. */ private hiddenForMovedIds = ''; - /** Set when the final reroute went out: the next model update carries its routes. */ - private revealOnNextModelUpdate = false; private revealTimer: ReturnType | undefined; handle(action: Action): void { - if (action.kind === UpdateModelAction.KIND || action.kind === SetModelAction.KIND) { - if (this.revealOnNextModelUpdate) { - this.revealHiddenEdges(); - } - return; - } if (action.kind !== MoveAction.KIND) { return; } @@ -236,15 +285,13 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { return; } dragHiddenEdgeIds.set(getDragKey(root), incidentEdgeIds); - this.revealOnNextModelUpdate = false; } - /** The hidden edges wait for the server's routes from now on, or for the fallback. */ + /** From the final reroute on, the hidden edges may wait this long for a route; then they are drawn as they are. */ private armReveal(): void { if (dragHiddenEdgeIds.size === 0) { return; } - this.revealOnNextModelUpdate = true; if (this.revealTimer) { clearTimeout(this.revealTimer); } @@ -256,7 +303,6 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { clearTimeout(this.revealTimer); this.revealTimer = undefined; } - this.revealOnNextModelUpdate = false; this.hiddenForMovedIds = ''; dragHiddenEdgeIds.clear(); } diff --git a/packages/diagram-client/src/stock-features.module.ts b/packages/diagram-client/src/stock-features.module.ts index 06afadf..5e6aba9 100644 --- a/packages/diagram-client/src/stock-features.module.ts +++ b/packages/diagram-client/src/stock-features.module.ts @@ -11,7 +11,7 @@ */ import { ContainerModule } from 'inversify'; import { ChangeBoundsTool, TYPES, bindAsService, configureActionHandler } from '@eclipse-glsp/client'; -import { MoveAction, SetModelAction, UpdateModelAction } from '@eclipse-glsp/sprotty'; +import { MoveAction } from '@eclipse-glsp/sprotty'; import { PropertyPanel } from './property-panel'; import { ChatPanel } from './chat-panel-integrated'; import { clientBehavior } from './profile'; @@ -152,11 +152,9 @@ export const workflowFeaturesModule = new ContainerModule((bind, unbind, isBound rebind(ChangeBoundsTool).to(WorkflowChangeBoundsTool).inSingletonScope(); // Drag-time rerouting: client streams MoveAction updates; server recomputes routes (ELK fixed). - // The moved node's edges are hidden through the drag and drawn again with - // the model update that carries the drop's routes. + // The moved node's edges are hidden through the drag and drawn again once + // a route for the node's new place is in the model (`isEdgeHiddenForDrag`). configureActionHandler(context, MoveAction.KIND, WorkflowElkLiveDragRouter); - configureActionHandler(context, UpdateModelAction.KIND, WorkflowElkLiveDragRouter); - configureActionHandler(context, SetModelAction.KIND, WorkflowElkLiveDragRouter); // The live tier: edges routed in the webview during a drag, with the same // router and the same anchors the server uses on commit, so the committed diff --git a/packages/diagram-client/src/views.ts b/packages/diagram-client/src/views.ts index 654ec9d..20f2059 100644 --- a/packages/diagram-client/src/views.ts +++ b/packages/diagram-client/src/views.ts @@ -1765,9 +1765,9 @@ export class WorkflowEdgeView extends PolylineEdgeView { // is not visible. That shared geometry is what made this possible: the // client router previously computed anchors differently, which is why // this code used to bypass the router registry entirely. - // An edge of a node being dragged is not drawn until the drop's routes - // arrive (see `isEdgeHiddenForDrag`). - if (isEdgeHiddenForDrag(rootOfEdge(edge), String((edge as any).id))) { + // An edge of a node being dragged is not drawn until a route for the + // node's new place arrives (see `isEdgeHiddenForDrag`). + if (isEdgeHiddenForDrag(rootOfEdge(edge), edge as any)) { return svg('g', { class: { 'workflow-edge': true, 'edge-hidden-for-drag': true } }); } const serverRoute = (edge as any).routingPoints as { x: number; y: number }[] | undefined; diff --git a/packages/diagram-client/test/container-parity.test.ts b/packages/diagram-client/test/container-parity.test.ts index df8cc79..a087eb5 100644 --- a/packages/diagram-client/test/container-parity.test.ts +++ b/packages/diagram-client/test/container-parity.test.ts @@ -32,8 +32,7 @@ * states, and the read-only view marker (hides the palette's tools while the view shown is * read-only, bound to itself and as a model-root listener), and the selection trace (its listener, * bound to itself and as a selection listener, and the `SelectionTraceFeedbackCommand` that draws - * the path), and the drag router as the `updateModel` and `setModel` handler (it draws a dragged - * node's edges again when the drop's routes arrive) — plus one change: `label:boundary:type` is bound to + * the path) — plus one change: `label:boundary:type` is bound to * the non-editable `BoundaryLabel`, because editing it renamed the port. * The oracle still does its job: it fails on any binding this composition gains * or loses, and updating the fixture is the deliberate act of accepting one. diff --git a/packages/diagram-client/test/edges-hidden-while-dragged.test.ts b/packages/diagram-client/test/edges-hidden-while-dragged.test.ts index 35dfebd..a922353 100644 --- a/packages/diagram-client/test/edges-hidden-while-dragged.test.ts +++ b/packages/diagram-client/test/edges-hidden-while-dragged.test.ts @@ -1,32 +1,42 @@ /** * The edges of a node being dragged are not drawn while it moves: they come - * back with the model update that carries the drop's routes. A move that is - * finished as it arrives (an arrow key) hides nothing. + * back as soon as their stored route reaches the ports again, which is the + * first model update with a route for the node's new place, or the node + * going back where it was. A move that is finished as it arrives (an arrow + * key) hides nothing. */ import 'reflect-metadata'; import { describe, expect, it, vi } from 'vitest'; import { WorkflowElkLiveDragRouter, isEdgeHiddenForDrag } from '../src/elk-live-drag-router'; -type Element = { id: string; type: string; parent?: Element; children?: Element[]; sourceId?: string; targetId?: string }; +type Point = { x: number; y: number }; +type Element = { + id: string; type: string; parent?: Element; children?: Element[]; + position?: Point; size?: { width: number; height: number }; + sourceId?: string; targetId?: string; routingPoints?: Point[]; +}; -/** a →(a-t)→ t, b →(b-t)→ t, c →(c-d)→ d, each node with an in and an out port. */ +/** 140x60 nodes with a 7x7 input port on the left and an output port on the right; an output anchor is (x+140, y+29.5), an input one (x, y+29.5). */ function model(): { root: any; elements: Map } { const elements = new Map(); const root: any = { id: 'root', type: 'graph', children: [] as Element[] }; - const node = (id: string): void => { - const n: Element = { id, type: 'node:actor', parent: root, children: [] }; - for (const port of ['in', 'out']) { - const p: Element = { id: `${id}.${port}`, type: port === 'in' ? 'port:input' : 'port:output', parent: n }; + const node = (id: string, x: number, y: number): void => { + const n: Element = { id, type: 'node:actor', parent: root, children: [], position: { x, y }, size: { width: 140, height: 60 } }; + for (const [port, px] of [['in', 0], ['out', 133]] as Array<[string, number]>) { + const p: Element = { id: `${id}.${port}`, type: port === 'in' ? 'port:input' : 'port:output', parent: n, position: { x: px, y: 26 }, size: { width: 7, height: 7 } }; n.children!.push(p); elements.set(p.id, p); } root.children.push(n); elements.set(id, n); }; - const edge = (from: string, to: string): void => { - const e: Element = { id: `${from}-${to}`, type: 'edge:connection', parent: root, sourceId: `${from}.out`, targetId: `${to}.in` }; + const edge = (from: string, to: string, route: Point[]): void => { + const e: Element = { id: `${from}-${to}`, type: 'edge:connection', parent: root, sourceId: `${from}.out`, targetId: `${to}.in`, routingPoints: route }; root.children.push(e); elements.set(e.id, e); }; - for (const id of ['a', 'b', 't', 'c', 'd']) { node(id); } - edge('a', 't'); edge('b', 't'); edge('c', 'd'); + node('a', 0, 0); node('b', 0, 200); node('t', 400, 100); node('c', 0, 400); node('d', 400, 400); + const into = (from: Point, to: Point): Point[] => [from, { x: 270, y: from.y }, { x: 270, y: to.y }, to]; + edge('a', 't', into({ x: 140, y: 29.5 }, { x: 400, y: 129.5 })); + edge('b', 't', into({ x: 140, y: 229.5 }, { x: 400, y: 129.5 })); + edge('c', 'd', [{ x: 140, y: 429.5 }, { x: 400, y: 429.5 }]); root.index = { getById: (id: string) => elements.get(id), all: () => elements.values() }; return { root, elements }; } @@ -39,63 +49,72 @@ function router(root: any): { handler: WorkflowElkLiveDragRouter; dispatched: an return { handler, dispatched }; } -const move = (id: string, finished = false): any => ({ kind: 'move', moves: [{ elementId: id, toPosition: { x: 10, y: 10 } }], finished }); -const hidden = (root: any, ...ids: string[]): boolean[] => ids.map(id => isEdgeHiddenForDrag(root, id)); +/** The pointer moves the node, as sprotty's feedback does, and the drag router hears of it. */ +function drag(m: ReturnType, handler: any, id: string, to: Point, finished = false): void { + m.elements.get(id)!.position = to; + handler.handle({ kind: 'move', moves: [{ elementId: id, toPosition: to }], finished }); +} +const hidden = (m: ReturnType, ...ids: string[]): boolean[] => ids.map(id => isEdgeHiddenForDrag(m.root, m.elements.get(id)! as any)); describe('edges hidden while their node is dragged', () => { it('hides the moved node\'s edges through the drag, not the others', () => { - const { root } = model(); - const { handler } = router(root); - handler.handle(move('t')); - expect(hidden(root, 'a-t', 'b-t', 'c-d')).toEqual([true, true, false]); - handler.handle(move('t')); - expect(hidden(root, 'a-t', 'b-t', 'c-d')).toEqual([true, true, false]); - handler.handle({ kind: 'updateModel' }); - handler.handle({ kind: 'updateModel' }); - handler.handle({ kind: 'setModel' }); - expect(hidden(root, 'a-t', 'b-t')).toEqual([true, true]); + const m = model(); + const { handler } = router(m.root); + drag(m, handler, 't', { x: 400, y: 120 }); + expect(hidden(m, 'a-t', 'b-t', 'c-d')).toEqual([true, true, false]); + drag(m, handler, 't', { x: 400, y: 160 }); + expect(hidden(m, 'a-t', 'b-t', 'c-d')).toEqual([true, true, false]); }); - it('draws them again with the model update after the drop\'s reroute went out', async () => { + it('draws an edge again as soon as a route for the node\'s new place is in the model', async () => { vi.useFakeTimers(); try { - const { root } = model(); - const { handler, dispatched } = router(root); - handler.handle(move('t')); - handler.handle(move('t', true)); - // The update that answers the ChangeBounds, before the reroute is sent, does not reveal. - handler.handle({ kind: 'updateModel' }); - expect(hidden(root, 'a-t')).toEqual([true]); + const m = model(); + const { handler, dispatched } = router(m.root); + drag(m, handler, 't', { x: 400, y: 160 }); + drag(m, handler, 't', { x: 400, y: 160 }, true); + expect(hidden(m, 'a-t', 'b-t')).toEqual([true, true]); + // The server's route for one edge arrives (whichever round trip brings it). + m.elements.get('a-t')!.routingPoints = [{ x: 140, y: 29.5 }, { x: 270, y: 29.5 }, { x: 270, y: 189.5 }, { x: 400, y: 189.5 }]; + expect(hidden(m, 'a-t', 'b-t')).toEqual([false, true]); await vi.advanceTimersByTimeAsync(100); expect(dispatched.length).toBe(1); - expect(hidden(root, 'a-t')).toEqual([true]); - handler.handle({ kind: 'updateModel' }); - expect(hidden(root, 'a-t', 'b-t')).toEqual([false, false]); + m.elements.get('b-t')!.routingPoints = [{ x: 140, y: 229.5 }, { x: 270, y: 229.5 }, { x: 270, y: 189.5 }, { x: 400, y: 189.5 }]; + expect(hidden(m, 'a-t', 'b-t')).toEqual([false, false]); } finally { vi.useRealTimers(); } }); - it('draws them as they were when no routes arrive', async () => { + it('draws them again at once when the drag is cancelled and the node goes back', () => { + const m = model(); + const { handler } = router(m.root); + drag(m, handler, 't', { x: 400, y: 160 }); + expect(hidden(m, 'a-t')).toEqual([true]); + m.elements.get('t')!.position = { x: 400, y: 100 }; + expect(hidden(m, 'a-t', 'b-t')).toEqual([false, false]); + }); + + it('draws them as they were when no route arrives', async () => { vi.useFakeTimers(); try { - const { root } = model(); - const { handler } = router(root); - handler.handle(move('t')); - handler.handle(move('t', true)); + const m = model(); + const { handler } = router(m.root); + drag(m, handler, 't', { x: 400, y: 160 }); + drag(m, handler, 't', { x: 400, y: 160 }, true); await vi.advanceTimersByTimeAsync(100); - expect(hidden(root, 'a-t')).toEqual([true]); + expect(hidden(m, 'a-t')).toEqual([true]); await vi.advanceTimersByTimeAsync(1600); - expect(hidden(root, 'a-t')).toEqual([false]); + expect(hidden(m, 'a-t')).toEqual([false]); } finally { vi.useRealTimers(); } }); it('hides nothing for a move by arrow key', () => { - const { root } = model(); - const { handler } = router(root); - handler.handle(move('t', true)); - expect(hidden(root, 'a-t', 'b-t')).toEqual([false, false]); + const m = model(); + const { handler } = router(m.root); + drag(m, handler, 't', { x: 400, y: 101 }, true); + expect(hidden(m, 'a-t', 'b-t')).toEqual([false, false]); }); }); diff --git a/packages/diagram-client/test/fixtures/container-parity.baseline.json b/packages/diagram-client/test/fixtures/container-parity.baseline.json index b558ca9..53c75c5 100644 --- a/packages/diagram-client/test/fixtures/container-parity.baseline.json +++ b/packages/diagram-client/test/fixtures/container-parity.baseline.json @@ -481,15 +481,5 @@ "id": "Symbol(stub.ISelectionListener)", "op": "bind", "to": "service:SelectionTraceListener" - }, - { - "handler": "WorkflowElkLiveDragRouter", - "kind": "updateModel", - "op": "actionHandler" - }, - { - "handler": "WorkflowElkLiveDragRouter", - "kind": "setModel", - "op": "actionHandler" } ] diff --git a/packages/diagram-client/test/glsp-stub.ts b/packages/diagram-client/test/glsp-stub.ts index 17a70c2..0139bc0 100644 --- a/packages/diagram-client/test/glsp-stub.ts +++ b/packages/diagram-client/test/glsp-stub.ts @@ -30,8 +30,6 @@ export const ApplyLabelEditOperation = { /** Drag-time reroute trigger — the base container binds a handler on this kind. */ export const MoveAction = { KIND: 'move' }; -export const UpdateModelAction = { KIND: 'updateModel' }; -export const SetModelAction = { KIND: 'setModel' }; export const NavigateToExternalTargetAction = { KIND: 'navigateToExternalTarget', diff --git a/packages/diagram-server/src/operations/reroute-edges-avoid-overlaps-handler.ts b/packages/diagram-server/src/operations/reroute-edges-avoid-overlaps-handler.ts index 7bcde50..09a4888 100644 --- a/packages/diagram-server/src/operations/reroute-edges-avoid-overlaps-handler.ts +++ b/packages/diagram-server/src/operations/reroute-edges-avoid-overlaps-handler.ts @@ -1020,6 +1020,7 @@ export class WorkflowRerouteEdgesAvoidOverlapsOperationHandler extends Operation private async executeOperation(operation: WorkflowRerouteEdgesAvoidOverlapsOperation, timing?: RerouteTiming): Promise { const nowMs = Date.now(); + const startedAt = nowMs; const src = operation.source ?? 'client'; this.modelState.set('workflow.lastRerouteAtMs', nowMs); this.modelState.set('workflow.lastRerouteSource', src); @@ -1138,6 +1139,7 @@ export class WorkflowRerouteEdgesAvoidOverlapsOperationHandler extends Operation console.info('[cal-reroute] slide — ' + slidEdgeIds.size + ' edges kept their routes'); } await this.persistRoutes(root, operation); + this.logPerf(operation, startedAt, slidEdgeIds.size, 0, 'slide'); return; } @@ -1262,6 +1264,14 @@ export class WorkflowRerouteEdgesAvoidOverlapsOperationHandler extends Operation } await this.persistRoutes(root, operation); + this.logPerf(operation, startedAt, slidEdgeIds.size, routed, usedLibavoid ? 'libavoid' : 'manhattan'); + } + + /** Always-on breadcrumb, like the layout's: what a persisted reroute did and how long it took. */ + private logPerf(operation: WorkflowRerouteEdgesAvoidOverlapsOperation, startedAt: number, kept: number, routed: number, router: string): void { + if (operation.preview) { return; } + // eslint-disable-next-line no-console + console.log(`[dialogram perf] reroute (${operation.source ?? 'client'}): ${kept} kept, ${routed} routed by ${router}, ${Date.now() - startedAt}ms`); } /** Persist the layout, positions and routes; not for a preview (a drag). */ From e888349ebcea933d90b5fbbda1b5415c445534d8 Mon Sep 17 00:00:00 2001 From: Endri Bezati Date: Thu, 5 Nov 2026 15:38:55 +0100 Subject: [PATCH 3/4] fix: a node held still mid-drag keeps its edges hidden A pause of more than 0.7s in a drag made the client's quiet timer take the drag for over: it sent the final reroute, armed the 1.5s fallback, and the fallback drew the edges again while the pointer was still down, where the client's live router drew them on their old routes. The quiet timer keeps its 0.7s only after a finished move; while the pointer is down it waits 5s, long enough to tell a node held still from a drag that never finishes. Claude-Session: https://claude.ai/code/session_01EToj6RJRMSSHQiqGNMU9wv --- CHANGELOG.md | 6 ++++-- .../diagram-client/src/elk-live-drag-router.ts | 12 ++++++++++-- .../test/edges-hidden-while-dragged.test.ts | 18 ++++++++++++++++++ 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55e9be1..9793d82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,8 +35,10 @@ platform API version. come back as soon as a route for the node's new place is in the model, as a schematic editor redraws a block's nets after it is placed; before, they trailed the pointer on their old routes and jumped at the drop. A move by - arrow key hides nothing, and a cancelled drag draws them again at once. If - no route arrives within 1.5s of the drop the edges are drawn as they were. + arrow key hides nothing, and a cancelled drag draws them again at once. A + node held still mid-drag keeps its edges hidden: a drag with no move for 5s + is taken as one that never finishes. If no route arrives within 1.5s of + the drop the edges are drawn as they were. - The server logs one `[dialogram perf] reroute` line per reroute it persists: edges kept, edges routed, the router and the time taken. diff --git a/packages/diagram-client/src/elk-live-drag-router.ts b/packages/diagram-client/src/elk-live-drag-router.ts index fe356aa..9c0170d 100644 --- a/packages/diagram-client/src/elk-live-drag-router.ts +++ b/packages/diagram-client/src/elk-live-drag-router.ts @@ -165,6 +165,10 @@ function routeReachesPorts(root: Readonly, edge: Readonly