[profiler] Draw a collapsed region nested inside an expanded one - #6947
Karakatiza666 wants to merge 1 commit into
Conversation
| const expand = selection.regionsExpanded.contains(topParent); | ||
| if (nodeId !== topParent && !expand) { | ||
| let set = result.nodeChildren.get(topParent); | ||
| // Find out whether we display this node or the innermost region that hides it. |
There was a problem hiding this comment.
drawnNode returns the outermost collapsed region, not the innermost — that is the whole point of the walk, and this line says the opposite of the doc comment three functions up. Suggest "the outermost collapsed region that hides it".
| static drawnNode(profile: CircuitProfile, selection: CircuitSelection, nodeId: NodeId): NodeId { | ||
| let regions: Array<NodeId> = []; | ||
| let parent = profile.parents.get(nodeId); | ||
| while (parent.isSome()) { | ||
| regions.unshift(parent.unwrap()); | ||
| parent = profile.parents.get(parent.unwrap()); | ||
| } | ||
| for (const region of regions) { | ||
| if (!selection.regionsExpanded.contains(region)) { | ||
| return region; | ||
| } | ||
| } | ||
| return nodeId; | ||
| } |
There was a problem hiding this comment.
"Outermost collapsed ancestor" is just the last collapsed node seen on a single upward walk — no array and no unshift (O(n) per insert) needed. This runs once per simple node and twice per edge, so it is worth keeping allocation-free:
| static drawnNode(profile: CircuitProfile, selection: CircuitSelection, nodeId: NodeId): NodeId { | |
| let regions: Array<NodeId> = []; | |
| let parent = profile.parents.get(nodeId); | |
| while (parent.isSome()) { | |
| regions.unshift(parent.unwrap()); | |
| parent = profile.parents.get(parent.unwrap()); | |
| } | |
| for (const region of regions) { | |
| if (!selection.regionsExpanded.contains(region)) { | |
| return region; | |
| } | |
| } | |
| return nodeId; | |
| } | |
| static drawnNode(profile: CircuitProfile, selection: CircuitSelection, nodeId: NodeId): NodeId { | |
| let drawn = nodeId; | |
| let parent = profile.parents.get(nodeId); | |
| while (parent.isSome()) { | |
| const region = parent.unwrap(); | |
| if (!selection.regionsExpanded.contains(region)) | |
| drawn = region; | |
| parent = profile.parents.get(region); | |
| } | |
| return drawn; | |
| } |
| // Every region around the drawn node is drawn too, expanded: cytoscape needs each | ||
| // `parent` it is given to be a node on the graph, up to the outermost one. | ||
| let ancestor = parent; | ||
| while (ancestor.isSome()) { | ||
| visibleParents.add(ancestor.unwrap()); | ||
| ancestor = profile.parents.get(ancestor.unwrap()); | ||
| } |
There was a problem hiding this comment.
This walk fixes a real dangling-parent bug, but no test in the PR covers it. I reverted it to visibleParents.add(parent.unwrap()) locally and all 178 tests still passed — nested() gives outer a direct simple child (n1), so outer is registered even without the walk.
A fixture with three levels (a > b > c > n2) where the outer regions have no direct simple child fails without it (b's parent is a, and a is not on the graph). Please add one, plus an invariant assertion that every parent referenced by a drawn node exists as a drawn node, across expansion states — that is exactly the cytoscape precondition the comment cites.
| set.unwrap().add(nodeId); | ||
| else { | ||
| result.nodeChildren.set(topParent, new Set(nodeId)); | ||
| result.nodeChildren.set(drawn, new Set([nodeId])); |
There was a problem hiding this comment.
new Set(nodeId) → new Set([nodeId]) is a genuine bug fix hiding in a refactor: nodeId is a string, so the old code splatted it into its characters (new Set('n2') = {'n','2'}), corrupting nodeChildren for the first hidden child of every collapsed region and therefore getSimpleNodes() / reachableFrom() — i.e. edge highlighting for collapsed regions. Reverting it leaves all 178 tests green, so it is untested; worth an assertion on nodeChildren and a line in the commit message, since it is not what the message describes.
| // Detect whether an edge represents the same channel as a previous edge | ||
| // (only suppressed if the edge goes to a complex node) | ||
| if (!expandTarget) { | ||
| if (targetCollapsed) { |
There was a problem hiding this comment.
targetCollapsed is not equivalent to the old !expandTarget for a target that sits in no region: getTopParent(t) === t and regionsExpanded.contains(t) was false for a simple node, so the old code deduped parallel edges into top-level operators; now it does not. Two n1 -> n2 edges between two region-less nodes render as two parallel edges instead of one (I confirmed: createEdge gives them n1->n2 and n1->n2:2).
That may well be the intent — the comment above says "only suppressed if the edge goes to a complex node" — but it is an unannounced behaviour change with no test. Please pin whichever behaviour you want.
|
Ran in
|
168cb4a to
8b215fc
Compare
8b215fc to
9647ae4
Compare
9647ae4 to
858e6ec
Compare
858e6ec to
0ece59f
Compare
0ece59f to
dbc6953
Compare
dbc6953 to
35bb864
Compare
35bb864 to
9aaef86
Compare
9aaef86 to
5ed0431
Compare
Which node stands in for a hidden operator was decided by `getTopParent`: the outermost region, consulted alone. Expanding it therefore revealed every descendant at once, and a region inside an expanded one could not be collapsed on its own. `drawnNode` walks the regions around a node from the outside in and returns the first collapsed one, or the node itself when all of them are expanded. Edges land on whichever region is drawn in place of their endpoints, and every region around a drawn node is now added to the graph: cytoscape requires each `parent` it is given to exist as a node. Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
5ed0431 to
9767527
Compare
Part of #6895, split one commit per PR.
Which node stands in for a hidden operator was decided by
getTopParent: the outermost region, consulted alone. Expanding ittherefore revealed every descendant at once, and a region inside an
expanded one could not be collapsed on its own.
drawnNodewalks the regions around a node from the outside in andreturns the first collapsed one, or the node itself when all of them are
expanded. Edges land on whichever region is drawn in place of their
endpoints, and every region around a drawn node is now added to the
graph: cytoscape requires each
parentit is given to exist as a node.Describe Manual Test Plan
Expand a region that holds another region. The inner one stays collapsed and can be expanded on its own, rather than the outer expansion revealing every descendant at once.
Verified at this commit, not just at the tip of the stack: checked out detached with
js-packages/profiler-lib/distdeleted and rebuilt from this commit's source, thenprofiler-libbun run checkandbun run test, andprofiler-layoutbun run checkandbun run test(all three vitest projects, browser suites included). All four green.Checklist
Breaking Changes?
Mark if you think the answer is yes for any of these components:
Describe Incompatible Changes
None. The change is confined to
js-packages/.