Skip to content

Only clusters with a non-None rect can become active parents during m… #155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hbmartin merged 1 commit into from
Jul 9, 2026
Merged

Only clusters with a non-None rect can become active parents during m… #155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions graphviz2drawio/graphviz2drawio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,14 @@ def _cluster_membership(
DOT allows a node to be referenced by several clusters even though Graphviz
renders it inside only one of them, so a membership candidate only counts
when the member's rendered rect actually lies inside the cluster's rect.
Names that fail to round-trip through the SVG titles simply leave the
member parented further up (ultimately at the page root), which keeps its
rendered position intact because child geometry is relativized against
whichever parent is chosen.
Names that fail to round-trip through the SVG titles, and clusters without
rendered rectangles, simply leave the member parented further up (ultimately
at the page root), which keeps its rendered position intact because child
geometry is relativized against whichever parent is chosen.
"""
rendered_cluster_names = set(clusters)
parentable_cluster_names = {
name for name, cluster in clusters.items() if cluster.rect is not None
}
rendered_node_names = set(nodes)
node_parents: dict[str, str] = {}
cluster_parents: dict[str, str] = {}
Expand All @@ -100,7 +102,7 @@ def _cluster_membership(
nodes,
clusters,
rendered_node_names,
rendered_cluster_names,
parentable_cluster_names,
node_parents,
cluster_parents,
)
Expand All @@ -113,13 +115,13 @@ def _walk_cluster_membership(
nodes: Mapping[str, Node],
clusters: Mapping[str, Node],
rendered_node_names: set[str],
rendered_cluster_names: set[str],
parentable_cluster_names: set[str],
node_parents: dict[str, str],
cluster_parents: dict[str, str],
) -> None:
for subgraph in subgraphs:
subgraph_name = str(subgraph.name) if subgraph.name is not None else None
if subgraph_name is not None and subgraph_name in rendered_cluster_names:
if subgraph_name is not None and subgraph_name in parentable_cluster_names:
current_cluster = subgraph_name
if parent_cluster is not None and _is_within_cluster(
clusters[subgraph_name].rect,
Expand All @@ -144,7 +146,7 @@ def _walk_cluster_membership(
nodes,
clusters,
rendered_node_names,
rendered_cluster_names,
parentable_cluster_names,
node_parents,
cluster_parents,
)
Expand Down
66 changes: 60 additions & 6 deletions scripts/drawio_export.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,26 @@
esac
}

render_timeout_command() {
case "${DRAWIO_RENDER_TIMEOUT:-300s}" in

Check failure on line 72 in scripts/drawio_export.sh

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Add a default case (*) to handle unexpected values. 


See more on https://sonarcloud.io/project/issues?id=hbmartin_graphviz2drawio&issues=AZ9HnlvdIKvy3GkUehAX&open=AZ9HnlvdIKvy3GkUehAX&pullRequest=155
0 | false | no)
return 1
;;
esac

if command -v timeout > /dev/null 2>&1; then
command -v timeout
return 0
fi

if command -v gtimeout > /dev/null 2>&1; then
command -v gtimeout
return 0
fi
Comment on lines +78 to +86

@gemini-code-assist gemini-code-assist Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

We can simplify the render_timeout_command function by avoiding running command -v twice for each command check. Instead, we can directly output the command name once the check succeeds.

Suggested change
if command -v timeout > /dev/null 2>&1; then
command -v timeout
return 0
fi
if command -v gtimeout > /dev/null 2>&1; then
command -v gtimeout
return 0
fi
if command -v timeout > /dev/null 2>&1; then
echo "timeout"
return 0
fi
if command -v gtimeout > /dev/null 2>&1; then
echo "gtimeout"
return 0
fi


return 1
}

render_drawio_png() {
local input_file="$1"
local output_file="$2"
Expand All @@ -76,8 +96,10 @@
local sandbox_status
local render_log
local render_status
local timeout_bin
local xvfb_status
local -a drawio_cmd
local -a timeout_cmd

drawio="$(resolve_drawio)" || return 1
drawio_cmd=("$drawio")
Expand All @@ -89,9 +111,22 @@
return 1
fi
fi
if [[ "$(uname -s)" == "Linux" ]]; then
drawio_cmd+=("--disable-dev-shm-usage" "--disable-gpu")
fi

timeout_cmd=()
if timeout_bin="$(render_timeout_command)"; then
timeout_cmd=(
"$timeout_bin"
--kill-after="${DRAWIO_RENDER_TIMEOUT_KILL_AFTER:-30s}"
"${DRAWIO_RENDER_TIMEOUT:-300s}"
)
fi

mkdir -p "$(dirname "$output_file")"
render_log="$(mktemp)"
echo "Rendering: $label -> $output_file"

if should_use_xvfb; then
xvfb_status=0
Expand All @@ -105,16 +140,32 @@
rm -f "$render_log"
return 1
fi
if xvfb-run -a "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then
render_status=0
if [[ "${#timeout_cmd[@]}" -gt 0 ]]; then
if "${timeout_cmd[@]}" xvfb-run -a "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then
render_status=0
else
render_status="$?"
fi
else
render_status="$?"
if xvfb-run -a "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then
render_status=0
else
render_status="$?"
fi
fi
elif [[ "$xvfb_status" -eq 1 ]]; then
if "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then
render_status=0
if [[ "${#timeout_cmd[@]}" -gt 0 ]]; then
if "${timeout_cmd[@]}" "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then
render_status=0
else
render_status="$?"
fi
else
render_status="$?"
if "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then
render_status=0
else
render_status="$?"
fi
fi
else
rm -f "$render_log"
Expand All @@ -128,6 +179,9 @@
fi

echo "Error: draw.io render failed for $label" >&2
if [[ "$render_status" -eq 124 || "$render_status" -eq 137 ]]; then
echo "draw.io render timed out after ${DRAWIO_RENDER_TIMEOUT:-300s}" >&2
fi
if [[ -s "$render_log" ]]; then
sed -n '1,40p' "$render_log" >&2
fi
Expand Down
73 changes: 73 additions & 0 deletions test/test_graphs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,79 @@ def test_cluster_membership_skips_cluster_without_rect() -> None:
assert cluster_parents == {}


def test_cluster_membership_uses_rendered_ancestor_for_no_rect_cluster() -> None:
graph = AGraph(
string=(
"digraph G {"
" subgraph cluster_parent {"
" subgraph cluster_missing {"
" subgraph cluster_inner { a }"
" b"
" }"
" }"
"}"
),
)
nodes = {
"a": SimpleNamespace(rect=Rect(25, 25, 5, 5)),
"b": SimpleNamespace(rect=Rect(50, 50, 5, 5)),
}
clusters = {
"cluster_parent": SimpleNamespace(rect=Rect(0, 0, 100, 100)),
"cluster_missing": SimpleNamespace(rect=None),
"cluster_inner": SimpleNamespace(rect=Rect(20, 20, 20, 20)),
}

node_parents, cluster_parents = graphviz2drawio._cluster_membership( # noqa: SLF001
graph,
nodes,
clusters,
)

assert node_parents["a"] == "cluster_inner"
assert node_parents["b"] == "cluster_parent"
assert cluster_parents == {"cluster_inner": "cluster_parent"}


def test_cells_by_plain_label_collapses_duplicate_labels() -> None:
"""Document a known limitation of the ``cells_by_plain_label`` test helper.

The helper keys cells by their rendered plain-text label, so two distinct
node cells that render the same text collapse to a single dict entry
(last-wins on insertion). Membership assertions built on this helper are
therefore only reliable for graphs whose visible labels are unique; a graph
with duplicate labels would silently drop a node and could mask a
regression. This test pins that behavior so the assumption is explicit.
"""
model = ElementTree.fromstring(
"""
<root>
<mxCell id="node_first" vertex="1" value="&lt;font&gt;dup&lt;/font&gt;" />
<mxCell id="node_second" vertex="1" value="&lt;font&gt;dup&lt;/font&gt;" />
<mxCell id="node_unique" vertex="1" value="&lt;font&gt;solo&lt;/font&gt;" />
</root>
""",
)
elements = list(model)

labels = cells_by_plain_label(elements)

# Both "dup" cells are real, distinct vertices...
assert (
len(
{
e.attrib["id"]
for e in elements
if e.attrib["value"].endswith("dup</font>")
},
)
== 2
)
# ...but they collapse to one entry, and the last one inserted wins.
assert set(labels) == {"dup", "solo"}
assert labels["dup"].attrib["id"] == "node_second"


def test_convnet() -> None:
file = "test/directed/convnet.gv.txt"
xml = graphviz2drawio.convert(file)
Expand Down
Loading