diff --git a/elk-task/elk-plan.md b/elk-task/elk-plan.md index eed483a8..bfb5543d 100644 --- a/elk-task/elk-plan.md +++ b/elk-task/elk-plan.md @@ -598,6 +598,172 @@ Each step keeps the extension working. unaffected in all arms. If the collateral ever needs to go, the untried route is a hierarchical child node for the frame column, which could carry its own fixed order without handing the whole graph to `semiInteractive`. +11. **Connected components are not separated.** A frame need not be connected to anything: + `factorial(n: int)` has one `int` local, so its node has no edges at all. With ELK's + default `separateConnectedComponents`, every such frame counted as a component of its + own, and the component packer arranged the components side by side to approximate an + aspect ratio. The result was a call stack laid out *horizontally*, marching rightwards + (frame x positions `36, 36, 176, 316`) until the Frames band overlapped the Objects + band by 162 px. `FIRST_SEPARATE` could not prevent it: the layer constraint applies + within a component, and each isolated frame was its own. + + `elk.separateConnectedComponents: false` lays the graph out as one unit, so the frame + layer holds every frame whether or not it has edges. It is not a trade: on + `example-showcase.py` crossings went *down*, 686 to 647, with layout time unchanged — + packing components separately was costing a little even where it did no visible harm. + + Worth knowing when reading a layout: frames in one layer are centred, not + left-aligned, so a wide `Global` and a narrow `factorial` have different x. Different + x alone does not mean different layers; compare the spans. +12. **The frame column stops jumping.** Reported as "size and positions of the frame list + jumps; better: fixed size, pinned to the left bottom". Measured over the 49 steps of + `example-showcase.py` at a 1100×530 viewport, the complaint had four independent + causes, and the fixes are listed below in the order they were found. + + | cause | evidence | + | --- | --- | + | frame width follows its content | 7 distinct widths, 120 → 196 px | + | ELK floats the frame layer vertically | gap below the stack takes 32 values, 0 → 383 px | + | auto-fit re-scales every step | scale oscillates, 16 changes of which 6 are zoom-*ins* | + | auto-fit rounds the scale up | content overflows the viewport on 11 of 49 steps | + + The useful metric is the *jerk*: the max and mean change in the frame column's screen + rect between consecutive steps. A count of distinct values conflates a slow drift, + which nobody notices, with a jump, which is the whole complaint. + + Four changes, each measured on its own: + + - **`FRAME_WIDTH_PX`** — frames are measured at a constant 220 px instead of shrinking + to their content. This also fixes their x for free, since ELK centres nodes within a + layer and equal widths therefore mean equal left edges. Only the measurement changes, + never the graph ELK is given, so it cannot reorder anything. Rows are `nowrap` with + an ellipsis, so a wider box clips rather than wraps and node heights are unchanged. + - **Zoom ratchet** — `autoFit` may lower the zoom but never raise it; the Fit button is + the way back. A heap that grows and shrinks by one object straddles a rung of the + zoom ladder, and re-fitting freely made the scale bounce 0.5 ↔ 0.33, resizing the + whole graph each time. Scale changes fell 16 → 4, zoom-ins 6 → 0. *Reverted in + §7.13 — `SIMPLE` placement removed most of what it was defending against.* + - **`floorStep`** — fitting picks the largest rung *at most* the required scale. + `nearestStep` rounded up about half the time, the content then overflowed, the + translate clamped, and the view lost its anchor. Overflow went 11/49 → 0/49. + - **`MIN_AUTO_FIT_SCALE = 0.5`** — auto-fit will not shrink further even when the graph + does not fit. The showcase ends 865×1070 in a 1100×530 panel — a portrait graph in a + landscape panel — so height decides the fit on every step, and a true ratio of 0.48 + rounded down to 0.33: a third of the scale lost to a rounding, leaving three quarters + of the width empty. Holding 0.5 hides 1% of the graph height on average and never + more than 5%. Floors further up stop being bargains — 0.67 hides 16%, 0.75 hides 21%. + The bottom anchor is deliberately unclamped so the overflow comes off the top, which + is empty space, rather than off the bottom, which is the frame column. + + That left the vertical float, and it turned out not to be a view problem at all but + `elk.layered.nodePlacement.strategy`. The default `BRANDES_KOEPF` aligns each node with + the nodes it is joined to, which on this shape — one frame column fanning out into a + heap — stretches the graph vertically and lets the frame column drift to wherever its + edge partners pull it. All five strategies, same 49 measured graphs: + + | strategy | gap jump, mean | gap max | height mean | crossings | stack order | ms | + | --- | --- | --- | --- | --- | --- | --- | + | `BRANDES_KOEPF` (default) | 65 px | 412 | 727 | 445 | 20/20 | 2815 | + | **`SIMPLE`** | **18 px** | **185** | **594** | **439** | **20/20** | **2325** | + | `LINEAR_SEGMENTS` | 29 px | 334 | 653 | 469 | 20/20 | 2637 | + | `NETWORK_SIMPLEX` | 33 px | 355 | 649 | 441 | 20/20 | 2385 | + | `INTERACTIVE` | 35 px | 391 | 600 | 467 | 20/20 | 2167 | + + `SIMPLE` packs each layer rather than aligning across layers, and wins on every axis at + once: less jitter, a shorter graph — which then fits at a larger zoom — fewer crossings + and less time. Nothing is traded away. + + **Two dead ends, both ruled out by measurement.** Neither should be tried again. + + - *Shifting the frames to the canvas bottom after layout.* Geometrically sound: the + stack pinned exactly (`gapsBelowStack: [0,1]`) and the edge patch was well founded — + of 262 frame-sourced edges across the showcase, all 231 that bend start with a + horizontal segment followed by a vertical one. But crossings tripled, 647 → 1921. + ELK assigns each edge a vertical lane in the gutter based on where it placed the + nodes, so moving a node afterwards invalidates every lane assignment. This rules out + the whole "lay out, then move" family. + - *An invisible spacer node atop the frame layer,* sized to push the stack to the + floor before ELK places and routes. Sizing it from the frames' summed heights was + simply wrong — Brandes-Köpf spreads frames apart to meet their targets, so the layer + is far taller than its nodes add up to. Sizing it from the *measured* gap worked on + synthetic shapes but not on the real graph: Brandes-Köpf aligns heap nodes with the + frames that point at them, so pushing the frames down drags the heap down too, the + graph grows by roughly what the spacer added, and the gap re-opens. It is a chase, + not a fixed point, so iterating only inflates further. Cost on the showcase: +81% + layout time (70 → 127 ms mean, cold) and +24% canvas height, for no change in + crossings. Dropping Brandes-Köpf removed the need entirely. + + Also checked and rejected as having no effect on the gap: node `alignment: BOTTOM` + under every Brandes-Köpf variant, and `contentAlignment: V_BOTTOM`. ELK has no native + bottom-align for a layer. + + Net effect over the 49 steps, start of the work to end: + + | step-to-step | before | after | + | --- | --- | --- | + | column left, mean Δ | 11.7 px | **0 px** | + | column width, mean Δ | 2.8 px | **0 px** | + | column bottom, mean Δ | 46.6 px | **8.8 px** | + | column bottom, max Δ | 245 px | **33 px** | + | crossings | 679 | **437** | + | canvas height, max | 1102 px | **928 px** | + | zoom at the final step | 0.33 | **0.5** | + | steps overflowing the viewport | 11/49 | **0/49** | + | cold layout, mean | 70 ms | **66 ms** | + + The column is horizontally frozen and vertically within ±33 px at worst. What remains + is cosmetic: the graph is portrait and the panel landscape, so width never binds and + the right of the panel stays empty. Only a layout that is wider and shorter could fill + it; zooming cannot, since the emptiness only closes at a scale that hides half the + graph. + +13. **The zoom ratchet comes back out.** + + The ratchet was measured under `BRANDES_KOEPF`. `SIMPLE` then made the graph 20% + shorter, so the premise was worth re-testing: a scale that moves less has less to + ratchet. Both arms re-measured on the current build, each from a fresh page load. + + Stepping forward 0 → 48, the ordinary way a trace is read, the two are **identical**: + + | forward sweep | ratchet | free | + | --- | --- | --- | + | scale changes | 2 | 2 | + | zoom-ins | 0 | 0 | + | scale sequence | 1 → 0.75 → 0.5 | 1 → 0.75 → 0.5 | + | column bottom, mean Δ | 10.8 px | 11.0 px | + | column top, mean Δ | 14.9 px | 14.4 px | + + This is not a coincidence: going forward the heap only grows, so the fitted scale only + falls, and a ratchet that only lets the scale fall never engages. The ratchet's entire + effect is on the way back. + + Over a there-and-back round trip (0 → 48 → 0, then 12 steps jiggled across the two + rungs of the ladder the showcase crosses — 110 transitions in all): + + | round trip | ratchet | free | + | --- | --- | --- | + | scale changes | 2 | 10 | + | zoom-ins | 0 | 4 | + | column left, mean Δ | 0.2 px | 0.8 px | + | column bottom, mean Δ | 11.6 px | 12.0 px | + | column bottom, max Δ | 43 px | 83 px | + | column top, mean Δ | 15.1 px | 19.1 px | + | column top, max Δ | 154 px | 307 px | + + So the cost is real but small — 2.5 px of mean column movement — and it is concentrated + in one behaviour: stepping back and forth across a rung makes the scale bounce + (0.75 → 1 → 0.75, 0.5 → 0.75 → 0.5), which is where the 83 px and 307 px maxima come + from. Against that, the ratchet's own cost is that one deep moment in a trace leaves the + graph shrunk for the whole of the rest of it, with no way back but the Fit button. A + reader stepping out of a recursion and finding the view still at half scale is worse + served than one who sees it grow back. The ratchet is removed; `fit` loses its + `allowZoomIn` parameter rather than keeping a flag nothing sets. + + The separate cap at 1 stays: fitting may zoom back in to undo an earlier shrink but + never magnifies past natural size, where a two-node graph would fill the panel. + + If the bounce ever becomes the complaint, the fix is hysteresis — zoom back in only + when the content clears the next rung by some margin — not a ratchet. ## 8. Decisions at a glance @@ -618,6 +784,10 @@ Each step keeps the extension working. | Neutral edges + hover highlighting instead of per-address hues | §6.4 | | Sizes measured from real DOM offscreen, not canvas text math | §6.2 | | Deterministic node order for step-to-step stability; no `considerModelOrder` | §6.5 | +| Node placement is `SIMPLE`, not the `BRANDES_KOEPF` default | §7.12 | +| Frames measured at a constant width; auto-fit floors at 0.5 | §7.12 | +| Auto-fit re-frames in both directions; the ratchet was measured and removed | §7.13 | +| The frame column is not pinned to the canvas bottom; both ways of doing it cost more than the float | §7.12 | | Cycle breaking stays `GREEDY`; no interactive layout seeding | §4, §6.5 | | elkjs pre-warmed at webview init to hide ~330 ms of JIT | §6.5 | | esbuild `minify` enabled before elkjs enters the bundle | §2 | diff --git a/pytrace-generator/main.py b/pytrace-generator/main.py index 97e7f749..e2ac57a3 100644 --- a/pytrace-generator/main.py +++ b/pytrace-generator/main.py @@ -47,11 +47,91 @@ def eprint(*args, **kwargs): set: "set" } -def primitive_type(t): +def union_member_str(value): + """ + One arm of a union, spelled the way CPython spells it inside `int | str`. + + Not `str(value)`, which gives "" for a class -- unions print the + module-qualified name instead, and leave the module off for builtins. + """ + if value is type(None): + # `Optional[int]` carries NoneType, which prints as "None" in a union. + return "None" + if isinstance(value, type): + if value.__module__ == "builtins": + return value.__qualname__ + return f"{value.__module__}.{value.__qualname__}" + return str(value) + + +def type_expression_str(value): + """ + How to spell `value` if it denotes a type rather than a value, or None if it does + not denote a type at all. + + Classes are the easy case. Everything else a type annotation can be built from -- + Union[...], Literal[...], Optional[...], int | str, list[int], Callable[...], + TypeVar, Any -- is an instance of a *private* typing class: typing._UnionGenericAlias, + typing._LiteralGenericAlias, types.UnionType and a dozen more. Looking those up in a + table would tie us to typing's internals and still miss whichever construct we forgot, + so they are recognised structurally and rendered with str(), which is how typing + spells them itself. + + Unions are the one exception, rendered by hand rather than with str(), because their + repr is not stable across the versions we support. 3.14 made `typing.Union` an alias + for `types.UnionType` (PEP 604), so `Union[int, float]` prints as "int | float" there + and "Union[int, float]" on 3.12 and 3.13 -- the same student program would be shown + differently depending on which interpreter happened to be running it. Everything is + spelled the 3.14 way, which is also how the annotation is usually written today. + """ + # Before the isinstance(value, type) check below: typing.Any is a class in 3.11+, and + # in 3.9/3.10 list[int] passed for one too. + origin = typing.get_origin(value) + if origin is not None or value is typing.Any or isinstance(value, typing.TypeVar): + # On 3.14 these two are the same object, so the second test is for 3.12 and 3.13, + # where `Union[int, str]` and `int | str` are still distinct kinds of thing. + if origin is types.UnionType or origin is typing.Union: + rendered = " | ".join(union_member_str(arg) for arg in typing.get_args(value)) + else: + rendered = str(value) + # "typing.Optional[int]" -> "Optional[int]" and + # "Union[__main__.Circle, ...]" -> "Union[Circle, ...]": neither prefix is how it + # is written in the source the student is looking at. Stripping __main__ here + # matches what type_name_regex already does for a bare class. + return rendered.replace("typing.", "").replace("__main__.", "") + if isinstance(value, typing.TypeAliasType): + # A PEP 695 alias, `type OnOff = Literal['on', 'off']`. Show what it stands for, + # not the fact that it is an alias: the name is already in the column next to it, + # so "" told the student nothing they could not see. + # + # The right-hand side is evaluated lazily, on first access, which is what makes + # `type Tree = Leaf | None` legal above the definition of Leaf. Until Leaf exists + # reading it raises NameError, and an alias we cannot spell out yet is still + # better shown as an alias than not at all. + try: + aliased = value.__value__ + except Exception: + return "" + # Recursion terminates: a recursive alias, `type Tree = int | list[Tree]`, holds + # the TypeAliasType object itself, which str()s to its bare name. + return type_expression_str(aliased) or str(aliased) + if isinstance(value, type): + # isinstance, not type(value) == type, or a class with a metaclass -- anything + # deriving from ABC, for one -- is not recognised as a class and ends up on the + # heap as an instance of ABCMeta. + type_name = str(value) + search_result = type_name_regex.search(type_name) + if search_result is not None: + type_name = f"" + return type_name + return None + + +def primitive_type(value): try: - return STACK_TYPES[t] + return STACK_TYPES[type(value)] except KeyError: - return "ref" + return "type" if type_expression_str(value) is not None else "ref" def complex_type(t): try: @@ -101,7 +181,7 @@ def format(self): class PrimitiveValue: def __init__(self, value, variable_name=None): self.variable_name = variable_name - self.type_str = primitive_type(type(value)) + self.type_str = primitive_type(value) if self.type_str == "ref": self.value = id(value) elif type(value) == float and math.isnan(value): @@ -123,14 +203,10 @@ def format(self): "type": self.type_str, "value": self.value } - if type(d["value"]) == type: - type_name = str(d["value"]) - search_result = type_name_regex.search(type_name) - if search_result is not None: - type_name = f"" - d["value"] = type_name - elif type(d["value"]) == typing.TypeAliasType: - d["value"] = "" + if self.type_str == "type": + # Classes, type aliases and type expressions alike: none of them survive + # json.dumps as themselves. + d["value"] = type_expression_str(self.value) elif inspect.isfunction(d["value"]): function_desc = str(d["value"]) search_result = function_str_regex.search(function_desc) @@ -303,14 +379,14 @@ def generate_heap(frame, script_path, ignore, return_value = None): for variable_name in frame.f_locals: if should_ignore_on_stack(variable_name, frame.f_locals[variable_name], script_path, ignore): continue - if primitive_type(type(frame.f_locals[variable_name])) != "ref": + if primitive_type(frame.f_locals[variable_name]) != "ref": continue value = frame.f_locals[variable_name] heap.store(id(value), value) if return_value is not None: # Store return value - if not should_ignore_on_stack("return", return_value, script_path) and not primitive_type(type(return_value)) != "ref": + if not should_ignore_on_stack("return", return_value, script_path) and not primitive_type(return_value) != "ref": heap.store(id(return_value), return_value) frame = frame.f_back diff --git a/pytrace-generator/test/test-cases/typeExpressions.py b/pytrace-generator/test/test-cases/typeExpressions.py new file mode 100644 index 00000000..44780163 --- /dev/null +++ b/pytrace-generator/test/test-cases/typeExpressions.py @@ -0,0 +1,33 @@ +from abc import ABC +from typing import Any, Callable, Literal, Optional, Union + + +class Meta(ABC): + pass + + +Color = Literal["red", "green"] +# The legacy spellings of a union. Both must come out in the `int | float` form, the +# same as `Modern` below: their repr changed in 3.14 and the expectation here is shared +# by every version the CI matrix runs. +Number = Union[int, float] +Modern = int | str +Maybe = Optional[int] +Items = list[int] +Fun = Callable[[int], str] +Anything = Any + +# PEP 695 aliases, which are shown unwrapped: what the alias stands for, not "". +type OnOff = Literal["on", "off"] +type Recursive = int | list[Recursive] +# Forward reference: the right-hand side is evaluated lazily, so this is unspellable +# until Leaf exists and is shown as "" for the steps before that. +type Tree = Leaf | None + + +class Leaf: + pass + + +c: Color = "red" +n: Number = 1 diff --git a/pytrace-generator/test/test-cases/typeExpressions.py.json b/pytrace-generator/test/test-cases/typeExpressions.py.json new file mode 100644 index 00000000..9c1daa83 --- /dev/null +++ b/pytrace-generator/test/test-cases/typeExpressions.py.json @@ -0,0 +1,745 @@ +[ + { + "line": 1, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 2, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 5, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 9, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [ + { + "type": "type", + "value": "", + "name": "Meta" + } + ] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 13, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [ + { + "type": "type", + "value": "", + "name": "Meta" + }, + { + "type": "type", + "value": "Literal['red', 'green']", + "name": "Color" + } + ] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 14, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [ + { + "type": "type", + "value": "", + "name": "Meta" + }, + { + "type": "type", + "value": "Literal['red', 'green']", + "name": "Color" + }, + { + "type": "type", + "value": "int | float", + "name": "Number" + } + ] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 15, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [ + { + "type": "type", + "value": "", + "name": "Meta" + }, + { + "type": "type", + "value": "Literal['red', 'green']", + "name": "Color" + }, + { + "type": "type", + "value": "int | float", + "name": "Number" + }, + { + "type": "type", + "value": "int | str", + "name": "Modern" + } + ] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 16, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [ + { + "type": "type", + "value": "", + "name": "Meta" + }, + { + "type": "type", + "value": "Literal['red', 'green']", + "name": "Color" + }, + { + "type": "type", + "value": "int | float", + "name": "Number" + }, + { + "type": "type", + "value": "int | str", + "name": "Modern" + }, + { + "type": "type", + "value": "int | None", + "name": "Maybe" + } + ] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 17, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [ + { + "type": "type", + "value": "", + "name": "Meta" + }, + { + "type": "type", + "value": "Literal['red', 'green']", + "name": "Color" + }, + { + "type": "type", + "value": "int | float", + "name": "Number" + }, + { + "type": "type", + "value": "int | str", + "name": "Modern" + }, + { + "type": "type", + "value": "int | None", + "name": "Maybe" + }, + { + "type": "type", + "value": "list[int]", + "name": "Items" + } + ] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 18, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [ + { + "type": "type", + "value": "", + "name": "Meta" + }, + { + "type": "type", + "value": "Literal['red', 'green']", + "name": "Color" + }, + { + "type": "type", + "value": "int | float", + "name": "Number" + }, + { + "type": "type", + "value": "int | str", + "name": "Modern" + }, + { + "type": "type", + "value": "int | None", + "name": "Maybe" + }, + { + "type": "type", + "value": "list[int]", + "name": "Items" + }, + { + "type": "type", + "value": "Callable[[int], str]", + "name": "Fun" + } + ] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 21, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [ + { + "type": "type", + "value": "", + "name": "Meta" + }, + { + "type": "type", + "value": "Literal['red', 'green']", + "name": "Color" + }, + { + "type": "type", + "value": "int | float", + "name": "Number" + }, + { + "type": "type", + "value": "int | str", + "name": "Modern" + }, + { + "type": "type", + "value": "int | None", + "name": "Maybe" + }, + { + "type": "type", + "value": "list[int]", + "name": "Items" + }, + { + "type": "type", + "value": "Callable[[int], str]", + "name": "Fun" + }, + { + "type": "type", + "value": "Any", + "name": "Anything" + } + ] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 22, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [ + { + "type": "type", + "value": "", + "name": "Meta" + }, + { + "type": "type", + "value": "Literal['red', 'green']", + "name": "Color" + }, + { + "type": "type", + "value": "int | float", + "name": "Number" + }, + { + "type": "type", + "value": "int | str", + "name": "Modern" + }, + { + "type": "type", + "value": "int | None", + "name": "Maybe" + }, + { + "type": "type", + "value": "list[int]", + "name": "Items" + }, + { + "type": "type", + "value": "Callable[[int], str]", + "name": "Fun" + }, + { + "type": "type", + "value": "Any", + "name": "Anything" + }, + { + "type": "type", + "value": "Literal['on', 'off']", + "name": "OnOff" + } + ] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 25, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [ + { + "type": "type", + "value": "", + "name": "Meta" + }, + { + "type": "type", + "value": "Literal['red', 'green']", + "name": "Color" + }, + { + "type": "type", + "value": "int | float", + "name": "Number" + }, + { + "type": "type", + "value": "int | str", + "name": "Modern" + }, + { + "type": "type", + "value": "int | None", + "name": "Maybe" + }, + { + "type": "type", + "value": "list[int]", + "name": "Items" + }, + { + "type": "type", + "value": "Callable[[int], str]", + "name": "Fun" + }, + { + "type": "type", + "value": "Any", + "name": "Anything" + }, + { + "type": "type", + "value": "Literal['on', 'off']", + "name": "OnOff" + }, + { + "type": "type", + "value": "int | list[Recursive]", + "name": "Recursive" + } + ] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 28, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [ + { + "type": "type", + "value": "", + "name": "Meta" + }, + { + "type": "type", + "value": "Literal['red', 'green']", + "name": "Color" + }, + { + "type": "type", + "value": "int | float", + "name": "Number" + }, + { + "type": "type", + "value": "int | str", + "name": "Modern" + }, + { + "type": "type", + "value": "int | None", + "name": "Maybe" + }, + { + "type": "type", + "value": "list[int]", + "name": "Items" + }, + { + "type": "type", + "value": "Callable[[int], str]", + "name": "Fun" + }, + { + "type": "type", + "value": "Any", + "name": "Anything" + }, + { + "type": "type", + "value": "Literal['on', 'off']", + "name": "OnOff" + }, + { + "type": "type", + "value": "int | list[Recursive]", + "name": "Recursive" + }, + { + "type": "type", + "value": "", + "name": "Tree" + } + ] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 32, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [ + { + "type": "type", + "value": "", + "name": "Meta" + }, + { + "type": "type", + "value": "Literal['red', 'green']", + "name": "Color" + }, + { + "type": "type", + "value": "int | float", + "name": "Number" + }, + { + "type": "type", + "value": "int | str", + "name": "Modern" + }, + { + "type": "type", + "value": "int | None", + "name": "Maybe" + }, + { + "type": "type", + "value": "list[int]", + "name": "Items" + }, + { + "type": "type", + "value": "Callable[[int], str]", + "name": "Fun" + }, + { + "type": "type", + "value": "Any", + "name": "Anything" + }, + { + "type": "type", + "value": "Literal['on', 'off']", + "name": "OnOff" + }, + { + "type": "type", + "value": "int | list[Recursive]", + "name": "Recursive" + }, + { + "type": "type", + "value": "Leaf | None", + "name": "Tree" + }, + { + "type": "type", + "value": "", + "name": "Leaf" + } + ] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 33, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [ + { + "type": "type", + "value": "", + "name": "Meta" + }, + { + "type": "type", + "value": "Literal['red', 'green']", + "name": "Color" + }, + { + "type": "type", + "value": "int | float", + "name": "Number" + }, + { + "type": "type", + "value": "int | str", + "name": "Modern" + }, + { + "type": "type", + "value": "int | None", + "name": "Maybe" + }, + { + "type": "type", + "value": "list[int]", + "name": "Items" + }, + { + "type": "type", + "value": "Callable[[int], str]", + "name": "Fun" + }, + { + "type": "type", + "value": "Any", + "name": "Anything" + }, + { + "type": "type", + "value": "Literal['on', 'off']", + "name": "OnOff" + }, + { + "type": "type", + "value": "int | list[Recursive]", + "name": "Recursive" + }, + { + "type": "type", + "value": "Leaf | None", + "name": "Tree" + }, + { + "type": "type", + "value": "", + "name": "Leaf" + }, + { + "type": "str", + "value": "red", + "name": "c" + } + ] + } + ], + "heap": {}, + "stdout": "" + }, + { + "line": 35, + "filePath": "typeExpressions.py", + "stack": [ + { + "frameName": "", + "locals": [ + { + "type": "type", + "value": "", + "name": "Meta" + }, + { + "type": "type", + "value": "Literal['red', 'green']", + "name": "Color" + }, + { + "type": "type", + "value": "int | float", + "name": "Number" + }, + { + "type": "type", + "value": "int | str", + "name": "Modern" + }, + { + "type": "type", + "value": "int | None", + "name": "Maybe" + }, + { + "type": "type", + "value": "list[int]", + "name": "Items" + }, + { + "type": "type", + "value": "Callable[[int], str]", + "name": "Fun" + }, + { + "type": "type", + "value": "Any", + "name": "Anything" + }, + { + "type": "type", + "value": "Literal['on', 'off']", + "name": "OnOff" + }, + { + "type": "type", + "value": "int | list[Recursive]", + "name": "Recursive" + }, + { + "type": "type", + "value": "Leaf | None", + "name": "Tree" + }, + { + "type": "type", + "value": "", + "name": "Leaf" + }, + { + "type": "str", + "value": "red", + "name": "c" + }, + { + "type": "int", + "value": 1, + "name": "n" + } + ] + } + ], + "heap": {}, + "stdout": "" + } +] \ No newline at end of file diff --git a/src/programflow-visualization/graph-model.ts b/src/programflow-visualization/graph-model.ts index d05c6a6d..25047aea 100644 --- a/src/programflow-visualization/graph-model.ts +++ b/src/programflow-visualization/graph-model.ts @@ -49,6 +49,23 @@ export type VizGraph = { nodes: Map; }; +/** + * Every frame is measured at this width instead of shrinking to its content. + * + * Content width moved with the variables on show -- seven widths between 120px and + * 196px over the 49 steps of the showcase -- so the Frames band resized under the + * reader's eye. A constant fixes the frames' x as well, for free: ELK centres nodes + * within their layer, so equal widths mean equal left edges. + * + * Only the measurement changes, never the graph ELK is handed, so this cannot move a + * node into a different layer or reorder anything: it widens one column and shifts the + * rest right. + * + * Rows are `nowrap` with an ellipsis, so a long value is clipped rather than wrapped + * and node heights stay at one line per row whatever the width. + */ +export const FRAME_WIDTH_PX = 220; + /** * Layout options settled by the spike, whose findings are in elk-task/elk-plan.md 7.1: * - cycleBreaking stays at its GREEDY default, or the frames' layer constraint throws @@ -57,6 +74,22 @@ export type VizGraph = { * (plan 6.5). Stability comes from emitting nodes in a deterministic order. * - semiInteractive is *not* here: it is switched on per step by buildGraph, and only * when there is more than one frame to order. See FRAME_ORDER_OPTIONS (plan 7.10). + * - separateConnectedComponents is off because a frame need not be connected to + * anything: a function whose locals are all primitives, `factorial(n: int)` being the + * classic, has no edges at all. ELK would treat each such frame as its own component + * and pack the components side by side, which lays the call stack out horizontally + * and pushes the Frames band across the Objects band (plan 7.11). + * - nodePlacement is SIMPLE rather than the BRANDES_KOEPF default. Brandes-Koepf lines + * each node up with the nodes it is joined to, which on this shape -- one frame + * column fanning out into a heap -- stretches the graph vertically and lets the frame + * column float: measured over the 49 steps of the showcase, the gap below the stack + * moved by 65 px between steps on average and reached 412 px. SIMPLE packs each layer + * instead, and beats the default on every measure that matters here: the gap moves + * 18 px between steps (-72%) and peaks at 185 px, the graph is 20% shorter so it + * fits the panel at a larger zoom, there are marginally fewer crossings (439 vs 445), + * layout is 17% quicker, and frame order is still correct on all 20 multi-frame + * steps. LINEAR_SEGMENTS, NETWORK_SIMPLEX and INTERACTIVE were measured too and all + * land between the two (plan 7.12). */ export const LAYOUT_OPTIONS: Record = { "elk.algorithm": "layered", @@ -64,6 +97,8 @@ export const LAYOUT_OPTIONS: Record = { "elk.edgeRouting": "ORTHOGONAL", "elk.spacing.nodeNode": "25", "elk.layered.spacing.nodeNodeBetweenLayers": "60", + "elk.separateConnectedComponents": "false", + "elk.layered.nodePlacement.strategy": "SIMPLE", }; /** diff --git a/src/programflow-visualization/web/measure.ts b/src/programflow-visualization/web/measure.ts index 01a81426..09a8c0fe 100644 --- a/src/programflow-visualization/web/measure.ts +++ b/src/programflow-visualization/web/measure.ts @@ -6,7 +6,7 @@ // measured size and the drawn size in agreement. import type { ElkNode } from "elkjs/lib/elk-api"; import type { VizGraph } from "../graph-model"; -import { inputPortId, keyPortId, rowPortId } from "../graph-model"; +import { FRAME_WIDTH_PX, inputPortId, keyPortId, rowPortId } from "../graph-model"; import { renderNode, headerElement, rowElements } from "./node-view"; const HOST_ID = "elk-measure-host"; @@ -61,6 +61,11 @@ export function measureGraph(viz: VizGraph): void { continue; } const element = renderNode(model); + if (model.address === undefined) { + // A frame. Width is fixed, and set here rather than after the read so the height + // that comes back is the height at that width. + element.style.width = `${FRAME_WIDTH_PX}px`; + } elements.set(child.id, element); host.append(element); } diff --git a/src/programflow-visualization/web/pan-zoom.ts b/src/programflow-visualization/web/pan-zoom.ts index 9b1a5ef5..95f4da42 100644 --- a/src/programflow-visualization/web/pan-zoom.ts +++ b/src/programflow-visualization/web/pan-zoom.ts @@ -12,12 +12,29 @@ const DRAG_THRESHOLD_PX = 3; const FIT_PADDING_PX = 16; +/** + * Auto-fitting will not shrink past this, even when the graph does not fit. + * + * Fitting is a means to reading, so shrinking below the point where the labels can be + * read defeats itself: it shows you all of something you cannot make out. The showcase + * ends 865x1070 in a 1100x530 panel -- a portrait graph in a landscape panel -- so the + * fit is decided by height on every one of its 49 steps, and the true ratio of 0.48 + * rounds *down* the ladder to 0.33. A third of the scale is lost to a rounding, and the + * result uses a quarter of the width available to it. + * + * Holding 0.5 costs, over those 49 steps, an average of 1% of the graph height off the + * top and never more than 5%. Floors further up the ladder stop being bargains: 0.67 + * hides 16% on average and 0.75 hides 21%. + */ +const MIN_AUTO_FIT_SCALE = 0.5; + export type PanZoom = { - /** Scale the content to fit the viewport and centre it, and re-enable auto-fitting. */ + /** Scale the content to fit the viewport, anchor it, and re-enable auto-fitting. */ fit(width: number, height: number): void; /** - * Same, but does nothing once the user has panned or zoomed. Until then every render - * re-frames the graph, which matters because a trace grows from one node to dozens. + * Same, but does nothing once the user has panned or zoomed. Until the user takes + * over, every render re-frames the graph, which matters because a trace grows from + * one node to dozens -- and shrinks back again as frames return. */ autoFit(width: number, height: number): void; /** One step up the zoom ladder, anchored on the middle of the viewport. */ @@ -37,6 +54,23 @@ function nearestStep(scale: number): number { ); } +/** + * The largest step that is still *at most* `scale`, so content scaled by it genuinely + * fits. Fitting used to pick the nearest step, which rounds up about half the time; + * the content then overflowed the viewport, the translate below clamped to 0, and the + * view lost its anchor. The smallest step wins when even that is too big -- at which + * point nothing can fit and overflowing is the only option left. + */ +function floorStep(scale: number): number { + let best = ZOOM_STEPS[0]; + for (const step of ZOOM_STEPS) { + if (step <= scale) { + best = step; + } + } + return best; +} + function stepFrom(scale: number, direction: 1 | -1): number { const index = ZOOM_STEPS.indexOf(nearestStep(scale)); return ZOOM_STEPS[Math.min(ZOOM_STEPS.length - 1, Math.max(0, index + direction))]; @@ -160,6 +194,20 @@ export function attachPanZoom(viewport: HTMLElement, canvas: HTMLElement): PanZo true ); + /** + * Re-frames in both directions: the scale may fall as the heap fills up and rise + * again as it empties. + * + * An earlier version was a ratchet that could only fall, on the theory that a heap + * straddling a boundary of the zoom ladder would make the scale oscillate. Measured + * on the showcase the theory barely holds any more, because SIMPLE node placement + * left the graph short enough for the scale to be stable: stepping 0 to 48 the two + * are *identical* -- the graph only grows, so a ratchet never engages -- and over a + * there-and-back round trip the free version costs 2.5px of mean frame-column + * movement. The oscillation is real but confined to stepping back and forth across + * one boundary, which costs a reader far less than a graph that stays shrunk for + * the rest of the trace once it has been big once (plan 7.13). + */ const fit = (width: number, height: number) => { const box = viewport.getBoundingClientRect(); if (width <= 0 || height <= 0 || box.width <= 0 || box.height <= 0) { @@ -171,10 +219,20 @@ export function attachPanZoom(viewport: HTMLElement, canvas: HTMLElement): PanZo (box.width - FIT_PADDING_PX) / width, (box.height - FIT_PADDING_PX) / height ); - // Never zoom *in* to fit: a two-node graph blown up to full width is unreadable. - scale = raw >= 1 ? 1 : nearestStep(raw); - translateX = Math.max(0, (box.width - width * scale) / 2); - translateY = Math.max(0, (box.height - height * scale) / 2); + // Capped at 1: fitting may zoom back in to undo an earlier shrink, but never + // magnifies past natural size, where a two-node graph would fill the panel. + scale = raw >= 1 ? 1 : Math.max(MIN_AUTO_FIT_SCALE, floorStep(raw)); + // Anchored bottom-left rather than centred. The frame column is the thing a reader + // keeps their eye on and it lives in the bottom-left corner of the graph, so that + // is the corner worth holding still. Centring handed it half of the graph's growth + // as displacement: the heap spreads right and down as it fills up, and the frames + // were pushed left and up by half of it on every step. + translateX = FIT_PADDING_PX / 2; + // Deliberately unclamped. Once the floor above lets content be taller than the + // panel, something has to go off-screen, and anchoring the bottom decides what: + // the overflow comes off the top, which is empty space and the far end of the heap, + // rather than off the bottom, which is the frame column. + translateY = box.height - FIT_PADDING_PX / 2 - height * scale; apply(); }; diff --git a/src/test/unit/graph-model.test.ts b/src/test/unit/graph-model.test.ts index ef539390..b15231f2 100644 --- a/src/test/unit/graph-model.test.ts +++ b/src/test/unit/graph-model.test.ts @@ -136,6 +136,37 @@ suite('graph-model: nodes', () => { assert.strictEqual(child.layoutOptions?.['elk.portConstraints'], 'FIXED_POS'); } }); + + test('a frame with no references still shares the frame layer', () => { + // factorial(n: int) has nothing but an int local, so its frame has no edges at all. + // With connected components separated, ELK treated each such frame as a component of + // its own and packed the components side by side, laying the call stack out + // horizontally and pushing the Frames band across the Objects band. + const elem = step( + [ + frame('', [local('p', ref(1))]), + frame('factorial', [local('n', int(3))]), + frame('factorial', [local('n', int(2))]), + ], + { 1: list() } + ); + const viz = build(elem); + assert.strictEqual( + viz.graph.layoutOptions?.['elk.separateConnectedComponents'], + 'false' + ); + // The isolated frames are still ordinary frame nodes in the same constrained layer. + for (const index of [0, 1, 2]) { + assert.strictEqual( + node(viz, frameNodeId(index)).layoutOptions?.['elk.layered.layering.layerConstraint'], + 'FIRST_SEPARATE' + ); + } + assert.deepStrictEqual( + viz.graph.edges?.flatMap((edge) => edge.sources), + ['frame:0:0'] + ); + }); }); suite('graph-model: rows', () => {