Skip to content

refactor(diagrams): apply the large-graph layout policy at the render point - #10

Merged
thomasmarsh merged 1 commit into
thomasmarsh:mainfrom
jimmykirk:diagram-layout-central
Aug 16, 2026
Merged

refactor(diagrams): apply the large-graph layout policy at the render point#10
thomasmarsh merged 1 commit into
thomasmarsh:mainfrom
jimmykirk:diagram-layout-central

Conversation

@jimmykirk

Copy link
Copy Markdown
Contributor

Follow-up to your review on #2 — you were right, and this is the version that does it once.

What changed

render_dot_to_svg is already the single place every graphviz object becomes
SVG. Its docstring says so, and it already carries one cross-cutting guarantee
(a render failure must never break the embedding page). The size policy is the
same kind of concern, so it now lives there and the seven per-builder calls are
gone. A new diagram type gets it without knowing it exists.

The exceptions had to become explicit

Deciding centrally means the cases that should not be re-laid out can no
longer be implied by which builders remembered to opt in:

  • Already force-directed graphs are left alone — render_calls uses fdp,
    render_heatmap uses sfdp. Checked via the engine, not a name list.
  • Ranks that carry meaning opt out via RANKED_BY_MEANING. Currently just
    the Hasse diagram, where the ranks are the partial order — force-directing
    it would destroy the thing it exists to show. This is the one case that
    genuinely needs a name, since "my ranks are semantic" is not observable from
    the graph.

It also fixes a real bug I shipped in #2

The merged version sets overlap="prism" — precisely the unsupported default
render_dot_to_svg's own docstring warns against:

never overlap="prism", which needs a triangulation library many distro
packages (Homebrew, several Linux distros) omit and reliably fails with
"remove_overlap: Graphviz not built with triangulation library"

So on a graphviz build without that library, every large diagram would have
fallen back to the placeholder SVG. It is now overlap="false", matching
render_calls. There is a test asserting prism is never requested.

Tests

Builder-level cases now exercise fit_layout_to_size directly rather than
asserting builders self-modify, plus new cases for both opt-outs and the prism
regression. FakeDot grows the attributes the render path now inspects, with a
comment explaining why.

26 passed against 23 on main for the same selection. The 17 errors in that
run are pre-existing in my environment (fixtures shelling out to cabal).

Note

CI on this will fail in Set up GHC like everything else at the moment — #9 has
the diagnosis, and it is unrelated to any of these changes.

… point

Follow-up to thomasmarsh#2, addressing the review: the helper had to be called by
every builder, so a new diagram type would silently miss it.

'render_dot_to_svg' is already the codebase's single point for turning a
graphviz object into SVG -- its docstring says every render must go
through it, and it already carries one cross-cutting guarantee (a render
failure must never break the embedding page). The size policy belongs in
the same place, so this moves it there and drops the seven per-builder
calls.

Deciding centrally means the exceptions have to be explicit rather than
implied by which builders remembered to opt in:

  * graphs that already chose a force-directed engine are left alone
    ('render_calls' uses fdp, 'render_heatmap' sfdp)
  * graphs whose ranks carry meaning opt out by name via
    RANKED_BY_MEANING -- currently the Hasse diagram, where the ranks are
    the partial order and a force-directed layout would destroy what it
    exists to show

Also fixes a real bug in the merged version: it set overlap="prism",
which is exactly the unsupported default 'render_dot_to_svg' warns
about -- prism needs a triangulation library many graphviz builds omit,
and would have failed the same way the docstring describes. Now
overlap="false", matching 'render_calls'.

Tests: the builder-level cases now exercise fit_layout_to_size directly,
plus new cases for the two opt-outs and one asserting prism is never
requested. FakeDot grows the attributes the render path now inspects.
26 passed against 23 on main for the same selection; the 17 errors are
pre-existing here (fixtures shelling out to cabal).
@thomasmarsh
thomasmarsh merged commit d88b9bb into thomasmarsh:main Aug 16, 2026
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants