From 62fe1e123a0663b63508e089ce44fc6284dec75d Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Thu, 3 Sep 2026 13:34:55 +0530 Subject: [PATCH 01/17] chore: fix clippy lints flagged by current stable `manual_filter` in viewer.rs fails `cargo clippy -- -D warnings` on the current stable toolchain and blocks the CI clippy job for every PR. Also fixes `useless_borrows_in_formatting` in the image.rs tests, which `cargo clippy --all-targets` reports. --- src/image.rs | 2 +- src/viewer.rs | 11 ++++------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/image.rs b/src/image.rs index 1593867..04360ea 100644 --- a/src/image.rs +++ b/src/image.rs @@ -3103,7 +3103,7 @@ mod tests { assert!( buf.windows(header_prefix.len()).any(|w| w == header_prefix), "header not found in output.\nGot (hex): {:?}", - &buf + buf ); // ── Check placeholder rows ──────────────────────────────────────── diff --git a/src/viewer.rs b/src/viewer.rs index caf14dd..43bcc3f 100644 --- a/src/viewer.rs +++ b/src/viewer.rs @@ -2639,13 +2639,10 @@ fn render_frame(stdout: &mut io::Stdout, state: &mut ViewerState) -> io::Result< let fill_bg = if is_json_cursor { Some(line_bg) } else { - line.spans.first().and_then(|s| s.style.bg).and_then(|bg| { - if line.spans.iter().all(|s| s.style.bg == Some(bg)) { - Some(bg) - } else { - None - } - }) + line.spans + .first() + .and_then(|s| s.style.bg) + .filter(|&bg| line.spans.iter().all(|s| s.style.bg == Some(bg))) }; if let Some(bg) = fill_bg { queue!( From f509b511e487afa079040922b5a650ce0232926c Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Thu, 3 Sep 2026 13:34:55 +0530 Subject: [PATCH 02/17] fix(diagram): route feedback edges around nodes and harden cycle handling Follow-up to #57 (refs #56). Feedback (back) edges now leave their source through the bottom border, travel along a gap row/column into a gutter lane, and enter the destination through its top (TD) or bottom (LR) border. Gap rows and columns never contain nodes, so a route can no longer pass through a sibling and read as an edge that does not exist. Self-loops render as closed loops. Lanes are assigned by span so longer edges wrap around shorter ones, sources or targets that share a layer use distinct rows/columns, and labels sit beside (TD) or inline on (LR) their own lane instead of overwriting each other. classify_feedback_edges uses an explicit stack instead of recursion, which overflowed the main-thread stack on long chains. The redundant edge_layer_feedback_indices pass is removed; both renderers share one layout() helper and one feedback set. LR forward edges now start flush with the right border of even-width nodes and bend at a column-based midpoint so junctions converge. Acyclic top-down output is byte-identical to before. Tests cover the acyclic example, TD/LR cycles and self-loops, sibling avoidance, shared layers, labels, a 200k-node chain, and run cycle renders under a timeout so a regression of the hang fails instead of hanging. --- src/diagram.rs | 1152 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 846 insertions(+), 306 deletions(-) diff --git a/src/diagram.rs b/src/diagram.rs index c4c3af7..3c7e98a 100644 --- a/src/diagram.rs +++ b/src/diagram.rs @@ -1,7 +1,7 @@ use crate::style::{Style, StyledSpan}; use crate::theme::Theme; use crossterm::style::Color; -use std::collections::{HashMap, HashSet, VecDeque}; +use std::collections::{BTreeSet, HashMap, HashSet, VecDeque}; // ───── Data types ───── @@ -276,6 +276,153 @@ pub(crate) struct NodeLayout { pub(crate) width: usize, } +impl NodeLayout { + /// Column of the left border character. + fn left_x(&self) -> usize { + self.center_x.saturating_sub(self.width / 2) + } + + /// Column of the right border character. + fn right_x(&self) -> usize { + self.left_x() + self.width.saturating_sub(1) + } + + /// Row of the bottom border character. + fn bottom_y(&self) -> usize { + self.top_y + 2 + } +} + +/// Output of the layering phase, shared by both renderers. +struct Layout { + /// Nodes grouped by layer; each layer is ordered left-to-right (TD) or + /// top-to-bottom (LR). + layers: Vec>, + /// `(layer index, position within layer)` for every node. + node_pos: HashMap, + /// Indices into `graph.edges` of the feedback (back) edges, sorted by the + /// number of layers they span, shortest first. Gutter lane `k` carries + /// `feedback[k]`, so the shortest edge sits innermost and longer edges wrap + /// around it instead of crossing it. + feedback: Vec, +} + +fn layout(graph: &Graph) -> Layout { + let feedback_set = classify_feedback_edges(graph); + let mut layers = assign_layers(graph, &feedback_set); + order_within_layers(&mut layers, graph, &feedback_set); + + let mut node_pos: HashMap = HashMap::new(); + for (layer_idx, layer) in layers.iter().enumerate() { + for (pos, id) in layer.iter().enumerate() { + node_pos.insert(id.clone(), (layer_idx, pos)); + } + } + + let span = |idx: usize| { + let edge = &graph.edges[idx]; + match (node_pos.get(&edge.from), node_pos.get(&edge.to)) { + (Some(&(from, _)), Some(&(to, _))) => from.abs_diff(to), + _ => 0, + } + }; + let mut feedback: Vec = feedback_set.into_iter().collect(); + feedback.sort_by_key(|&idx| (span(idx), idx)); + + Layout { + layers, + node_pos, + feedback, + } +} + +/// Routing decisions for one feedback edge. +struct FeedbackPlan { + /// Index into `graph.edges`. + edge: usize, + /// Gutter lane, 0 = innermost. + lane: usize, + /// Rank of the source among the feedback sources in its layer, counted + /// from the gutter side (0 = nearest the gutter). Sources that share a + /// layer leave on different rows or columns so their routes do not merge. + src_rank: usize, + /// Same for the destination among the feedback targets in its layer. + dst_rank: usize, +} + +fn plan_feedback(graph: &Graph, layout: &Layout) -> Vec { + let mut sources: HashMap> = HashMap::new(); + let mut targets: HashMap> = HashMap::new(); + for &idx in &layout.feedback { + let edge = &graph.edges[idx]; + if let Some(&(layer, pos)) = layout.node_pos.get(&edge.from) { + sources.entry(layer).or_default().insert(pos); + } + if let Some(&(layer, pos)) = layout.node_pos.get(&edge.to) { + targets.entry(layer).or_default().insert(pos); + } + } + let rank = |set: &HashMap>, layer: usize, pos: usize| { + set.get(&layer) + .map_or(0, |positions| positions.range(pos + 1..).count()) + }; + + layout + .feedback + .iter() + .enumerate() + .filter_map(|(lane, &idx)| { + let edge = &graph.edges[idx]; + let &(src_layer, src_pos) = layout.node_pos.get(&edge.from)?; + let &(dst_layer, dst_pos) = layout.node_pos.get(&edge.to)?; + Some(FeedbackPlan { + edge: idx, + lane, + src_rank: rank(&sources, src_layer, src_pos), + dst_rank: rank(&targets, dst_layer, dst_pos), + }) + }) + .collect() +} + +/// Geometry of one feedback edge route in a top-down diagram. +/// See [`Canvas::draw_feedback_edge_td`]. +pub(crate) struct FeedbackRouteTd { + /// Column where the route leaves the source's bottom border. + pub(crate) exit_x: usize, + /// Row of the source's bottom border. + pub(crate) src_bottom_y: usize, + /// Gap row of the horizontal run below the source. + pub(crate) exit_y: usize, + /// Column where the arrowhead enters the destination's top border. + pub(crate) entry_x: usize, + /// Row of the destination's top border. + pub(crate) dst_top_y: usize, + /// Gap row of the horizontal run above the destination. + pub(crate) entry_y: usize, + /// Column of the vertical lane in the gutter right of the diagram. + pub(crate) lane_x: usize, +} + +/// Geometry of one feedback edge route in a left-right diagram. +/// See [`Canvas::draw_feedback_edge_lr`]. +pub(crate) struct FeedbackRouteLr { + /// Column where the route leaves the source's bottom border. + pub(crate) exit_x: usize, + /// Row of the source's bottom border. + pub(crate) src_bottom_y: usize, + /// Gap column right of the source that carries the drop to the lane. + pub(crate) exit_lane_x: usize, + /// Column where the arrowhead enters the destination's bottom border. + pub(crate) entry_x: usize, + /// Row of the destination's bottom border. + pub(crate) dst_bottom_y: usize, + /// Gap column left of the destination that carries the rise from the lane. + pub(crate) entry_lane_x: usize, + /// Row of the horizontal lane in the gutter below the diagram. + pub(crate) lane_y: usize, +} + fn assign_layers(graph: &Graph, feedback_edges: &HashSet) -> Vec> { // Build a DAG view by excluding feedback edges. let node_ids = graph_node_ids(graph); @@ -325,7 +472,9 @@ fn assign_layers(graph: &Graph, feedback_edges: &HashSet) -> Vec) -> Vec> = vec![Vec::new(); max_layer + 1]; for &node in &topo_order { let layer = node_layer[node]; - // Return owned IDs to keep the original layout helper signature. layers[layer].push(node.to_string()); } layers.retain(|l| !l.is_empty()); @@ -440,6 +588,13 @@ fn graph_node_ids(graph: &Graph) -> Vec<&str> { ids } +/// Find the edges that must be set aside to make the graph acyclic. +/// +/// Runs a depth-first search from every node in declaration order and reports +/// each edge that points at a node still on the search path (a back edge), +/// plus every self-loop. Removing those edges leaves a DAG, which is what the +/// layer assignment needs. The search keeps its own explicit stack so that a +/// long chain of nodes cannot overflow the native call stack. fn classify_feedback_edges(graph: &Graph) -> HashSet { #[derive(Clone, Copy)] enum VisitState { @@ -447,36 +602,9 @@ fn classify_feedback_edges(graph: &Graph) -> HashSet { Visited, } - fn visit<'a>( - node: &'a str, - adj: &HashMap<&'a str, Vec<(usize, &'a str)>>, - visit_set: &mut HashMap<&'a str, VisitState>, - feedback: &mut HashSet, - ) { - visit_set.insert(node, VisitState::Visiting); - - for &(edge_idx, to) in adj.get(node).into_iter().flatten() { - if to == node { - // Self-loops are feedback edges and cannot participate in DAG layering. - feedback.insert(edge_idx); - continue; - } - - match visit_set.get(to).copied() { - None => visit(to, adj, visit_set, feedback), - Some(VisitState::Visiting) => { - // Edges to nodes still on the DFS stack are feedback edges. - feedback.insert(edge_idx); - } - Some(VisitState::Visited) => {} - } - } - - visit_set.insert(node, VisitState::Visited); - } - + let node_ids = graph_node_ids(graph); let mut adj: HashMap<&str, Vec<(usize, &str)>> = HashMap::new(); - for id in graph_node_ids(graph) { + for &id in &node_ids { adj.entry(id).or_default(); } for (idx, edge) in graph.edges.iter().enumerate() { @@ -486,42 +614,47 @@ fn classify_feedback_edges(graph: &Graph) -> HashSet { adj.entry(edge.to.as_str()).or_default(); } - let mut visit_set: HashMap<&str, VisitState> = HashMap::new(); + let mut state: HashMap<&str, VisitState> = HashMap::new(); let mut feedback = HashSet::new(); - for id in graph_node_ids(graph) { - if !visit_set.contains_key(id) { - visit(id, &adj, &mut visit_set, &mut feedback); - } - } + // Each frame holds a node and the index of its next unexplored out-edge. + let mut stack: Vec<(&str, usize)> = Vec::new(); - feedback -} + for &root in &node_ids { + if state.contains_key(root) { + continue; + } + state.insert(root, VisitState::Visiting); + stack.push((root, 0)); + + while let Some(frame) = stack.last_mut() { + let (node, next) = *frame; + let edges = &adj[node]; + if next >= edges.len() { + state.insert(node, VisitState::Visited); + stack.pop(); + continue; + } + frame.1 += 1; -fn edge_layer_feedback_indices( - graph: &Graph, - layers: &[Vec], - feedback_edges: &HashSet, -) -> Vec { - let mut node_layer: HashMap<&str, usize> = HashMap::new(); - for (layer_idx, layer) in layers.iter().enumerate() { - for id in layer { - node_layer.insert(id.as_str(), layer_idx); + let (edge_idx, to) = edges[next]; + if to == node { + feedback.insert(edge_idx); + continue; + } + match state.get(to) { + None => { + state.insert(to, VisitState::Visiting); + stack.push((to, 0)); + } + Some(VisitState::Visiting) => { + feedback.insert(edge_idx); + } + Some(VisitState::Visited) => {} + } } } - graph - .edges - .iter() - .enumerate() - .filter_map(|(idx, edge)| { - let is_feedback = feedback_edges.contains(&idx) - || node_layer - .get(edge.from.as_str()) - .zip(node_layer.get(edge.to.as_str())) - .is_some_and(|(from, to)| from >= to); - is_feedback.then_some(idx) - }) - .collect() + feedback } fn node_box_width(node: &Node) -> usize { @@ -985,141 +1118,112 @@ impl Canvas { } } - #[allow(clippy::too_many_arguments)] - pub(crate) fn draw_feedback_edge_td( - &mut self, - src_right_x: usize, - src_cy: usize, - dst_right_x: usize, - dst_cy: usize, - lane_x: usize, - label: Option<&str>, - edge_fg: Option, - label_fg: Option, - ) { - if lane_x >= self.width || src_cy >= self.height || dst_cy >= self.height { - return; - } - - let src_start_x = src_right_x + 1; - let arrow_x = dst_right_x + 1; - if src_start_x >= self.width || arrow_x >= self.width { - return; + /// Draw a feedback (back) edge in a top-down diagram. + /// + /// The route leaves the source through its bottom border, runs right along + /// a gap row into a vertical lane beside the diagram, comes back along a + /// gap row above the destination and enters it through its top border: + /// + /// ```text + /// ┌───────┐ + /// │ │ + /// ▼ │ + /// ┌────────┐ │ + /// │ Dst │ │ + /// └────────┘ │ + /// │ │ + /// ▼ │ + /// ┌────────┐ │ + /// │ Src │ │ + /// └────────┘ │ + /// │ │ + /// └─────┘ + /// ``` + /// + /// Gap rows never contain nodes, so the route cannot pass through a + /// sibling of either endpoint. Forward edges it crosses render as + /// junctions. + pub(crate) fn draw_feedback_edge_td(&mut self, route: &FeedbackRouteTd, fg: Option) { + let r = route; + + // Leave the source: stem down to the gap row, turn, run right to the lane. + for y in (r.src_bottom_y + 1)..r.exit_y { + self.add_connection(r.exit_x, y, CONN_UP | CONN_DOWN, fg); } - - for x in src_start_x..lane_x { - self.add_connection(x, src_cy, CONN_LEFT | CONN_RIGHT, edge_fg); + self.add_connection(r.exit_x, r.exit_y, CONN_UP | CONN_RIGHT, fg); + for x in (r.exit_x + 1)..r.lane_x { + self.add_connection(x, r.exit_y, CONN_LEFT | CONN_RIGHT, fg); } + self.add_connection(r.lane_x, r.exit_y, CONN_LEFT | CONN_UP, fg); - let (min_y, max_y) = if src_cy < dst_cy { - (src_cy, dst_cy) - } else { - (dst_cy, src_cy) - }; - for y in (min_y + 1)..max_y { - self.add_connection(lane_x, y, CONN_UP | CONN_DOWN, edge_fg); - } - - if src_cy == dst_cy { - self.add_connection(lane_x, src_cy, CONN_LEFT, edge_fg); - } else { - let src_turn = if dst_cy < src_cy { - CONN_LEFT | CONN_UP - } else { - CONN_LEFT | CONN_DOWN - }; - let dst_turn = if dst_cy < src_cy { - CONN_LEFT | CONN_DOWN - } else { - CONN_LEFT | CONN_UP - }; - self.add_connection(lane_x, src_cy, src_turn, edge_fg); - self.add_connection(lane_x, dst_cy, dst_turn, edge_fg); + // Up the lane. + for y in (r.entry_y + 1)..r.exit_y { + self.add_connection(r.lane_x, y, CONN_UP | CONN_DOWN, fg); } - if arrow_x + 1 < lane_x { - for x in (arrow_x + 1)..lane_x { - self.add_connection(x, dst_cy, CONN_LEFT | CONN_RIGHT, edge_fg); - } + // Back across the gap row above the destination, then down into it. + self.add_connection(r.lane_x, r.entry_y, CONN_DOWN | CONN_LEFT, fg); + for x in (r.entry_x + 1)..r.lane_x { + self.add_connection(x, r.entry_y, CONN_LEFT | CONN_RIGHT, fg); } - self.set(arrow_x, dst_cy, '◀', edge_fg); - - if let Some(text) = label { - let label_y = min_y + (max_y - min_y) / 2; - for (i, ch) in text.chars().enumerate() { - self.set(lane_x + 2 + i, label_y, ch, label_fg); - } + self.add_connection(r.entry_x, r.entry_y, CONN_RIGHT | CONN_DOWN, fg); + let arrow_y = r.dst_top_y.saturating_sub(1); + for y in (r.entry_y + 1)..arrow_y { + self.add_connection(r.entry_x, y, CONN_UP | CONN_DOWN, fg); } + self.set(r.entry_x, arrow_y, '▼', fg); } - #[allow(clippy::too_many_arguments)] - pub(crate) fn draw_feedback_edge_lr( - &mut self, - src_cx: usize, - src_bottom_y: usize, - dst_cx: usize, - dst_bottom_y: usize, - lane_y: usize, - label: Option<&str>, - edge_fg: Option, - label_fg: Option, - ) { - if lane_y >= self.height || src_cx >= self.width || dst_cx >= self.width { - return; + /// Draw a feedback (back) edge in a left-right diagram. + /// + /// The route leaves the source through its bottom border, drops down the + /// gap column right of it into a horizontal lane below the diagram, runs + /// left, rises up the gap column left of the destination and enters it + /// through its bottom border: + /// + /// ```text + /// ┌───────┐ ┌───────┐ + /// │ Dst │─────▶│ Src │ + /// └───────┘ └───────┘ + /// ▲ └─┐ + /// ┌─┘ │ + /// └──────────────────────┘ + /// ``` + /// + /// Gap columns never contain nodes, so the route cannot pass through a + /// node stacked above or below either endpoint. + pub(crate) fn draw_feedback_edge_lr(&mut self, route: &FeedbackRouteLr, fg: Option) { + let r = route; + let exit_y = r.src_bottom_y + 1; + let entry_y = r.dst_bottom_y + 2; + + // Leave the source: turn under its bottom border, run right, drop. + self.add_connection(r.exit_x, exit_y, CONN_UP | CONN_RIGHT, fg); + for x in (r.exit_x + 1)..r.exit_lane_x { + self.add_connection(x, exit_y, CONN_LEFT | CONN_RIGHT, fg); } - - let arrow_y = dst_bottom_y + 1; - if arrow_y >= self.height { - return; + self.add_connection(r.exit_lane_x, exit_y, CONN_LEFT | CONN_DOWN, fg); + for y in (exit_y + 1)..r.lane_y { + self.add_connection(r.exit_lane_x, y, CONN_UP | CONN_DOWN, fg); } - for y in (src_bottom_y + 1)..lane_y { - self.add_connection(src_cx, y, CONN_UP | CONN_DOWN, edge_fg); + // Along the lane. + self.add_connection(r.exit_lane_x, r.lane_y, CONN_UP | CONN_LEFT, fg); + for x in (r.entry_lane_x + 1)..r.exit_lane_x { + self.add_connection(x, r.lane_y, CONN_LEFT | CONN_RIGHT, fg); } + self.add_connection(r.entry_lane_x, r.lane_y, CONN_UP | CONN_RIGHT, fg); - if src_cx == dst_cx { - self.add_connection(src_cx, lane_y, CONN_UP, edge_fg); - } else { - let (min_x, max_x) = if src_cx < dst_cx { - (src_cx, dst_cx) - } else { - (dst_cx, src_cx) - }; - for x in (min_x + 1)..max_x { - self.add_connection(x, lane_y, CONN_LEFT | CONN_RIGHT, edge_fg); - } - - let src_turn = if dst_cx < src_cx { - CONN_UP | CONN_LEFT - } else { - CONN_UP | CONN_RIGHT - }; - let dst_turn = if dst_cx < src_cx { - CONN_UP | CONN_RIGHT - } else { - CONN_UP | CONN_LEFT - }; - self.add_connection(src_cx, lane_y, src_turn, edge_fg); - self.add_connection(dst_cx, lane_y, dst_turn, edge_fg); + // Rise beside the destination, run right under it, then up into it. + for y in (entry_y + 1)..r.lane_y { + self.add_connection(r.entry_lane_x, y, CONN_UP | CONN_DOWN, fg); } - - for y in (arrow_y + 1)..lane_y { - self.add_connection(dst_cx, y, CONN_UP | CONN_DOWN, edge_fg); - } - self.set(dst_cx, arrow_y, '▲', edge_fg); - - if let Some(text) = label { - let (min_x, max_x) = if src_cx < dst_cx { - (src_cx, dst_cx) - } else { - (dst_cx, src_cx) - }; - let label_x = min_x + (max_x - min_x).saturating_sub(text.chars().count()) / 2; - let label_y = lane_y.saturating_sub(1); - for (i, ch) in text.chars().enumerate() { - self.set(label_x + i, label_y, ch, label_fg); - } + self.add_connection(r.entry_lane_x, entry_y, CONN_DOWN | CONN_RIGHT, fg); + for x in (r.entry_lane_x + 1)..r.entry_x { + self.add_connection(x, entry_y, CONN_LEFT | CONN_RIGHT, fg); } + self.add_connection(r.entry_x, entry_y, CONN_LEFT | CONN_UP, fg); + self.set(r.entry_x, r.dst_bottom_y + 1, '▲', fg); } pub(crate) fn to_span_rows(&self, theme: &Theme) -> Vec> { @@ -1164,17 +1268,12 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz let edge_gap: usize = 4; let h_gap: usize = 4; - let feedback_edges = classify_feedback_edges(graph); - let mut layers = assign_layers(graph, &feedback_edges); - order_within_layers(&mut layers, graph, &feedback_edges); - let routed_feedback_edges = edge_layer_feedback_indices(graph, &layers, &feedback_edges); - let routed_feedback_set: HashSet = routed_feedback_edges.iter().copied().collect(); - let max_feedback_label_width = routed_feedback_edges - .iter() - .filter_map(|idx| graph.edges[*idx].label.as_ref()) - .map(|label| label.chars().count()) - .max() - .unwrap_or(0); + let layout = layout(graph); + let layers = &layout.layers; + if layers.is_empty() { + return None; + } + let plans = plan_feedback(graph, &layout); // Calculate node widths let mut widths: HashMap = HashMap::new(); @@ -1184,7 +1283,7 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz // Find widest layer to determine canvas width let mut max_layer_width: usize = 0; - for layer in &layers { + for layer in layers { let w: usize = layer .iter() .map(|id| widths.get(id).copied().unwrap_or(7)) @@ -1193,18 +1292,51 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz max_layer_width = max_layer_width.max(w); } - let core_canvas_width = max_layer_width + 6; // margin on each side - let feedback_gutter_width = if routed_feedback_edges.is_empty() { + let core_width = max_layer_width + 6; // margin on each side + let core_height = layers.len() * (node_height + edge_gap) - edge_gap; + let last_layer = layers.len() - 1; + + // A feedback edge leaves its source below the box and enters its + // destination above the box. The source nearest the gutter turns on the + // second gap row, which keeps its route clear of a straight-edge label on + // the first; every other source turns on the first gap row and passes + // under its right-hand siblings. Entries mirror this above the destination. + // Two sources (or two targets) in one layer therefore never share a row. + let exit_rows_below = |src_rank: usize| if src_rank == 0 { 2 } else { 1 }; + let entry_rows_above = |dst_rank: usize| if dst_rank == 0 { 3 } else { 4 }; + + // Routes into the first layer or out of the last one need extra rows. + let mut top_margin = 0usize; + let mut bottom_margin = 0usize; + for plan in &plans { + let edge = &graph.edges[plan.edge]; + if let Some(&(layer, _)) = layout.node_pos.get(&edge.to) + && layer == 0 + { + top_margin = top_margin.max(entry_rows_above(plan.dst_rank)); + } + if let Some(&(layer, _)) = layout.node_pos.get(&edge.from) + && layer == last_layer + { + bottom_margin = bottom_margin.max(exit_rows_below(plan.src_rank)); + } + } + + let max_label_width = plans + .iter() + .filter_map(|plan| graph.edges[plan.edge].label.as_ref()) + .map(|label| label.chars().count()) + .max() + .unwrap_or(0); + // Lanes sit four columns apart, plus room for a label beside each lane. + let lane_gap = 4 + max_label_width; + let gutter_width = if plans.is_empty() { 0 } else { - routed_feedback_edges.len() * 4 + max_feedback_label_width + 4 + lane_gap * plans.len() }; - let canvas_width = core_canvas_width + feedback_gutter_width; - let canvas_height = layers.len() * (node_height + edge_gap) - edge_gap; - - if canvas_height == 0 { - return None; - } + let canvas_width = core_width + gutter_width; + let canvas_height = top_margin + core_height + bottom_margin; let mut canvas = Canvas::new(canvas_width, canvas_height); @@ -1215,10 +1347,10 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz // First pass: calculate centers for the widest layer // Then align single-node layers to the canvas center - let canvas_center = core_canvas_width / 2; + let canvas_center = core_width / 2; for (layer_idx, layer) in layers.iter().enumerate() { - let y = layer_idx * (node_height + edge_gap); + let y = top_margin + layer_idx * (node_height + edge_gap); // Compute node centers relative to layer, then offset to center in canvas let node_widths_in_layer: Vec = layer @@ -1260,39 +1392,57 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz } } - // Draw edges let edge_fg = Some(theme.code_border); let label_fg = Some(theme.h3); // Use a distinct color for edge labels - let mut feedback_lane = 0usize; + // Feedback edges go first so that forward-edge arrowheads and labels, + // which overwrite cells, end up on top of any route they touch. + let lane_x = |lane: usize| core_width + 1 + lane * lane_gap; + let mut labels: Vec<(usize, usize, &str)> = Vec::new(); + for plan in &plans { + let edge = &graph.edges[plan.edge]; + let (Some(src), Some(dst)) = (positions.get(&edge.from), positions.get(&edge.to)) else { + continue; + }; + let route = FeedbackRouteTd { + exit_x: src.right_x().saturating_sub(1), + src_bottom_y: src.bottom_y(), + exit_y: src.bottom_y() + exit_rows_below(plan.src_rank), + entry_x: dst.right_x().saturating_sub(1), + dst_top_y: dst.top_y, + entry_y: dst.top_y.saturating_sub(entry_rows_above(plan.dst_rank)), + lane_x: lane_x(plan.lane), + }; + canvas.draw_feedback_edge_td(&route, edge_fg); + + if let Some(text) = edge.label.as_deref() { + // Beside this edge's lane, level with the middle of its vertical run. + let y = (route.entry_y + route.exit_y) / 2; + labels.push((route.lane_x + 2, y, text)); + } + } + for (x, y, text) in labels { + for (i, ch) in text.chars().enumerate() { + canvas.set(x + i, y, ch, label_fg); + } + } + + // Forward edges + let feedback_set: HashSet = layout.feedback.iter().copied().collect(); for (idx, edge) in graph.edges.iter().enumerate() { + if feedback_set.contains(&idx) { + continue; + } if let (Some(src), Some(dst)) = (positions.get(&edge.from), positions.get(&edge.to)) { - if routed_feedback_set.contains(&idx) { - let lane_x = core_canvas_width + 1 + feedback_lane * 4; - feedback_lane += 1; - canvas.draw_feedback_edge_td( - src.center_x + src.width / 2, - src.top_y + 1, - dst.center_x + dst.width / 2, - dst.top_y + 1, - lane_x, - edge.label.as_deref(), - edge_fg, - label_fg, - ); - } else { - let src_bottom = src.top_y + 2; - let dst_top = dst.top_y; - canvas.draw_edge_td( - src.center_x, - src_bottom, - dst.center_x, - dst_top, - edge.label.as_deref(), - edge_fg, - label_fg, - ); - } + canvas.draw_edge_td( + src.center_x, + src.bottom_y(), + dst.center_x, + dst.top_y, + edge.label.as_deref(), + edge_fg, + label_fg, + ); } } @@ -1306,12 +1456,14 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz let node_height: usize = 3; let node_h_gap: usize = 6; // horizontal gap between columns for edge routing let v_gap: usize = 2; // vertical gap between nodes in same column + let lane_gap: usize = 2; // rows between gutter lanes - let feedback_edges = classify_feedback_edges(graph); - let mut layers = assign_layers(graph, &feedback_edges); - order_within_layers(&mut layers, graph, &feedback_edges); - let routed_feedback_edges = edge_layer_feedback_indices(graph, &layers, &feedback_edges); - let routed_feedback_set: HashSet = routed_feedback_edges.iter().copied().collect(); + let layout = layout(graph); + let layers = &layout.layers; + if layers.is_empty() { + return None; + } + let plans = plan_feedback(graph, &layout); // Calculate node widths let mut widths: HashMap = HashMap::new(); @@ -1335,17 +1487,14 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz let canvas_width: usize = col_widths.iter().sum::() + (layers.len().saturating_sub(1)) * node_h_gap + 4; - let core_canvas_height = max_nodes_in_layer * (node_height + v_gap) - v_gap + 2; - let feedback_gutter_height = if routed_feedback_edges.is_empty() { + let core_height = max_nodes_in_layer * (node_height + v_gap) - v_gap + 2; + // One spare row under the diagram, then a lane every `lane_gap` rows. + let gutter_height = if plans.is_empty() { 0 } else { - routed_feedback_edges.len() * 2 + 2 + lane_gap * plans.len() + 1 }; - let canvas_height = core_canvas_height + feedback_gutter_height; - - if canvas_height == 0 { - return None; - } + let canvas_height = core_height + gutter_height; let mut canvas = Canvas::new(canvas_width, canvas_height); @@ -1353,12 +1502,15 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz let border_fg = Some(theme.code_border); let text_fg = Some(theme.fg); + // (left, right) border columns of each layer's column. + let mut col_bounds: Vec<(usize, usize)> = Vec::with_capacity(layers.len()); let mut col_x = 2; // starting x with margin for (layer_idx, layer) in layers.iter().enumerate() { let col_w = col_widths[layer_idx]; + col_bounds.push((col_x, col_x + col_w - 1)); let total_layer_height = layer.len() * node_height + layer.len().saturating_sub(1) * v_gap; - let start_y = (core_canvas_height.saturating_sub(total_layer_height)) / 2; + let start_y = (core_height.saturating_sub(total_layer_height)) / 2; for (node_idx, id) in layer.iter().enumerate() { let w = widths.get(id).copied().unwrap_or(7); @@ -1382,44 +1534,79 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz col_x += col_w + node_h_gap; } - // Draw edges let edge_fg = Some(theme.code_border); let label_fg = Some(theme.h3); - let mut feedback_lane = 0usize; + // Feedback edges go first so that forward-edge arrowheads and labels, + // which overwrite cells, end up on top of any route they touch. + let lane_y = |lane: usize| core_height + 1 + lane * lane_gap; + let mut labels: Vec<(usize, usize, &str)> = Vec::new(); + for plan in &plans { + let edge = &graph.edges[plan.edge]; + let (Some(src), Some(dst)) = (positions.get(&edge.from), positions.get(&edge.to)) else { + continue; + }; + // The source nearest the gutter drops one column clear of its border + // and any other source two columns, so two sources stacked in one + // column use different gap columns and an upper source's drop does + // not hug the box below it. + let route = FeedbackRouteLr { + exit_x: src.right_x().saturating_sub(1), + src_bottom_y: src.bottom_y(), + exit_lane_x: src.right_x() + if plan.src_rank == 0 { 1 } else { 2 }, + entry_x: dst.left_x() + 1, + dst_bottom_y: dst.bottom_y(), + entry_lane_x: dst.left_x().saturating_sub(2), + lane_y: lane_y(plan.lane), + }; + canvas.draw_feedback_edge_lr(&route, edge_fg); + + if let Some(text) = edge.label.as_deref() { + // Inline on the lane, centered on its horizontal run. + let inner = route.exit_lane_x.saturating_sub(route.entry_lane_x + 1); + let x = route.entry_lane_x + 1 + inner.saturating_sub(text.chars().count()) / 2; + labels.push((x, route.lane_y, text)); + } + } + for (x, y, text) in labels { + for (i, ch) in text.chars().enumerate() { + canvas.set(x + i, y, ch, label_fg); + } + } + + // Forward edges + let feedback_set: HashSet = layout.feedback.iter().copied().collect(); for (idx, edge) in graph.edges.iter().enumerate() { + if feedback_set.contains(&idx) { + continue; + } if let (Some(src), Some(dst)) = (positions.get(&edge.from), positions.get(&edge.to)) { - if routed_feedback_set.contains(&idx) { - let lane_y = core_canvas_height + 1 + feedback_lane * 2; - feedback_lane += 1; - canvas.draw_feedback_edge_lr( - src.center_x, - src.top_y + 2, - dst.center_x, - dst.top_y + 2, - lane_y, - edge.label.as_deref(), - edge_fg, - label_fg, - ); - } else { - let src_right_x = src.center_x + src.width / 2; - let src_cy = src.top_y + 1; - let dst_left_x = dst.center_x.saturating_sub(dst.width / 2); - let dst_cy = dst.top_y + 1; - - canvas.draw_edge_lr( - src.center_x, - src_right_x, - src_cy, - dst_left_x, - dst_cy, - edge.label.as_deref(), - edge_fg, - label_fg, - None, - ); - } + // Bend in the middle of the gap between the two columns, so that + // every edge between them turns in the same column no matter how + // wide the individual nodes are. + let mid_x = match ( + layout.node_pos.get(&edge.from), + layout.node_pos.get(&edge.to), + ) { + (Some(&(src_layer, _)), Some(&(dst_layer, _))) => { + let src_right = col_bounds[src_layer].1; + let dst_left = col_bounds[dst_layer].0; + (dst_left > src_right + 1) + .then(|| src_right + 1 + (dst_left - src_right - 1) / 2) + } + _ => None, + }; + canvas.draw_edge_lr( + src.center_x, + src.right_x(), + src.top_y + 1, + dst.left_x(), + dst.top_y + 1, + edge.label.as_deref(), + edge_fg, + label_fg, + mid_x, + ); } } @@ -1442,60 +1629,413 @@ pub fn render_mermaid(code: &str, theme: &Theme) -> Option<(Vec> #[cfg(test)] mod tests { use super::*; + use std::sync::mpsc; + use std::thread; + use std::time::Duration; fn render_text(code: &str) -> String { let theme = Theme::dark(); let (rows, _) = render_mermaid(code, &theme).expect("diagram should render"); rows.into_iter() - .map(|row| row.into_iter().map(|span| span.text).collect::()) + .map(|row| { + row.into_iter() + .map(|span| span.text) + .collect::() + .trim_end() + .to_string() + }) .collect::>() .join("\n") } + /// Render on a helper thread so that a layout that never terminates fails + /// the test instead of hanging the whole test binary. + fn render_text_with_timeout(code: &'static str) -> String { + let (tx, rx) = mpsc::channel(); + thread::spawn(move || { + let _ = tx.send(render_text(code)); + }); + rx.recv_timeout(Duration::from_secs(10)) + .expect("rendering did not finish within 10 seconds") + } + + /// Compare a render against a snapshot written at column 0 inside a raw + /// string literal (one leading newline, trailing blank rows ignored). + fn assert_render(code: &'static str, expected: &str) { + let text = render_text_with_timeout(code); + let expected = expected.strip_prefix('\n').unwrap_or(expected); + assert_eq!( + text.trim_end(), + expected.trim_end(), + "\n--- rendered ---\n{text}\n--- expected ---\n{expected}" + ); + } + + /// A linear chain N0 -> N1 -> ... -> N{n-1}, optionally closed into a cycle. + fn chain_graph(n: usize, close_cycle: bool) -> Graph { + let ids: Vec = (0..n).map(|i| format!("N{i}")).collect(); + let nodes = ids + .iter() + .map(|id| { + ( + id.clone(), + Node { + label: id.clone(), + shape: NodeShape::Rectangle, + }, + ) + }) + .collect(); + let mut edges: Vec = ids + .windows(2) + .map(|pair| Edge { + from: pair[0].clone(), + to: pair[1].clone(), + label: None, + }) + .collect(); + if close_cycle { + edges.push(Edge { + from: ids[n - 1].clone(), + to: ids[0].clone(), + label: None, + }); + } + Graph { + direction: Direction::TopDown, + nodes, + edges, + node_order: ids, + } + } + + // ── Cycle classification ── + + #[test] + fn closing_edge_of_a_declared_cycle_is_the_feedback_edge() { + let graph = parse_mermaid("graph TD\n A --> B\n B --> C\n C --> A\n").unwrap(); + let feedback = classify_feedback_edges(&graph); + assert_eq!(feedback.into_iter().collect::>(), vec![2]); + } + + #[test] + fn deep_chain_does_not_overflow_the_stack() { + let acyclic = chain_graph(200_000, false); + assert!(classify_feedback_edges(&acyclic).is_empty()); + + let cyclic = chain_graph(200_000, true); + let feedback = classify_feedback_edges(&cyclic); + assert_eq!(feedback.len(), 1); + assert!(feedback.contains(&(cyclic.edges.len() - 1))); + } + + #[test] + fn longer_feedback_edges_take_outer_lanes() { + let graph = parse_mermaid( + "graph TD\n Start --> Check\n Check --> Work\n Work --> Done\n Done --> Start\n Work --> Check\n", + ) + .unwrap(); + let layout = layout(&graph); + // Work -> Check spans one layer, Done -> Start spans three. + assert_eq!(layout.feedback, vec![4, 3]); + } + + // ── Acyclic rendering must not change ── + + #[test] + fn acyclic_top_down_diagram() { + assert_render( + "graph TD\n A[Start] --> B{Decision}\n B -->|Yes| C[Action 1]\n B -->|No| D[Action 2]\n C --> E[End]\n D --> E\n", + r#" + ┌───────┐ + │ Start │ + └───────┘ + │ + │ + │ + ▼ + ◆────────────◆ + │ Decision │ + ◆────────────◆ + │ + Yes │ No + ┌───────┴───────┐ + ▼ ▼ + ┌──────────┐ ┌──────────┐ + │ Action 1 │ │ Action 2 │ + └──────────┘ └──────────┘ + │ │ + │ │ + └───────┬───────┘ + ▼ + ┌─────┐ + │ End │ + └─────┘ +"#, + ); + } + + // ── Cycles ── + + #[test] + fn top_down_cycle_wraps_around_the_right() { + assert_render( + "graph TB\n Loop --> Execute\n Execute --> Repeat\n Repeat --> Loop\n", + r#" + ┌───────┐ + │ │ + ▼ │ + ┌──────┐ │ + │ Loop │ │ + └──────┘ │ + │ │ + │ │ + │ │ + ▼ │ + ┌─────────┐ │ + │ Execute │ │ + └─────────┘ │ + │ │ + │ │ + │ │ + ▼ │ + ┌────────┐ │ + │ Repeat │ │ + └────────┘ │ + │ │ + └──────┘ +"#, + ); + } + + #[test] + fn left_right_cycle_wraps_underneath() { + assert_render( + "graph LR\n A[Start] --> B[End]\n B --> A\n", + r#" + + ┌───────┐ ┌─────┐ + │ Start │─────▶│ End │ + └───────┘ └─────┘ + ▲ └─┐ +┌──┘ │ +└───────────────────────┘ +"#, + ); + } + + #[test] + fn self_loop_top_down_is_a_closed_loop() { + assert_render( + "graph TD\n A[Self] --> A\n", + r#" + ┌─────┐ + │ │ + ▼ │ + ┌──────┐ │ + │ Self │ │ + └──────┘ │ + │ │ + └─────┘ +"#, + ); + } + + #[test] + fn self_loop_left_right_is_a_closed_loop() { + assert_render( + "graph LR\n A[Self] --> A\n", + r#" + + ┌──────┐ + │ Self │ + └──────┘ + ▲ └─┐ +┌──┘ │ +└─────────┘ +"#, + ); + } + + // ── Routes must not pass through other nodes ── + #[test] - fn renders_top_down_cycle_with_feedback_edge() { - let text = render_text( + fn feedback_edge_avoids_sibling_nodes_top_down() { + let code = "graph TD\n A --> B\n A --> C\n B --> D\n C --> D\n D --> B\n"; + let text = render_text(code); + // Nothing may be drawn between the two siblings on their middle row. + assert!(text.contains("│ B │ │ C │"), "{text}"); + assert_render( + code, r#" -graph TB - Loop --> Execute - Execute --> Repeat - Repeat --> Loop + ┌─────┐ + │ A │ + └─────┘ + │ + ┌───┼────────────┐ + ┌─┼───┴────┐ │ + ▼ ▼ ▼ │ + ┌─────┐ ┌─────┐ │ + │ B │ │ C │ │ + └─────┘ └─────┘ │ + │ │ │ + │ │ │ + └─────┬────┘ │ + ▼ │ + ┌─────┐ │ + │ D │ │ + └─────┘ │ + │ │ + └──────────┘ "#, ); + } + + #[test] + fn feedback_edge_avoids_stacked_nodes_left_right() { + let code = "graph LR\n A --> B\n A --> C\n B --> D\n C --> D\n D --> B\n"; + let text = render_text(code); + // C sits directly below B; the route into B must not cross C's box. + let c_col = text + .lines() + .find_map(|line| line.find("│ C │")) + .expect("C is drawn"); + for line in text.lines() { + let cell = line.chars().nth(c_col + 3).unwrap_or(' '); + assert!( + cell != '│' || line.contains("│ B │") || line.contains("│ C │"), + "line through the column of C:\n{text}" + ); + } + assert_render( + code, + r#" - assert!(text.contains("Loop")); - assert!(text.contains("Execute")); - assert!(text.contains("Repeat")); - assert!(text.contains('▼')); - assert!(text.contains('◀')); + ┌─────┐ + ┌─▶│ B │───┐ + ┌─────┐ │ └─────┘ │ ┌─────┐ + │ A │───┤ ▲ ├─▶│ D │ + └─────┘ │┌──┘ │ └─────┘ + ││ ┌─────┐ │ └─┐ + └┼▶│ C │───┘ │ + │ └─────┘ │ + │ │ + │ │ + └─────────────────────┘ +"#, + ); } #[test] - fn renders_self_loop_without_hanging() { - let text = render_text( + fn two_feedback_sources_in_one_layer_leave_on_different_rows() { + assert_render( + "graph TD\n S --> A\n S --> B\n A --> S\n B --> S\n", r#" -graph TD - A[Self] --> A + ┌──────────┬───┐ + │ │ │ + ▼ │ │ + ┌─────┐ │ │ + │ S │ │ │ + └─────┘ │ │ + │ │ │ + │ │ │ + ┌─────┴────┐ │ │ + ▼ ▼ │ │ + ┌─────┐ ┌─────┐ │ │ + │ A │ │ B │ │ │ + └─────┘ └─────┘ │ │ + └──────────┼─────┘ │ + └─────────┘ "#, ); + } + + #[test] + fn two_feedback_sources_in_one_column_drop_in_different_columns() { + assert_render( + "graph LR\n S --> A\n S --> B\n A --> S\n B --> S\n", + r#" - assert!(text.contains("Self")); - assert!(text.contains('◀')); + ┌─────┐ + ┌─▶│ A │ + ┌─────┐ │ └─────┘ + │ S │───┤ └──┐ + └─────┘ │ │ + ▲ │ ┌─────┐ │ +┌──┘ └─▶│ B │ │ +│ └─────┘ │ +│ └─┐│ +│ ││ +├─────────────────────┼┘ +│ │ +└─────────────────────┘ +"#, + ); } + // ── Labels ── + #[test] - fn renders_left_right_cycle_with_feedback_edge() { - let text = render_text( + fn feedback_labels_sit_beside_their_own_lane_top_down() { + let code = "graph TD\n Start --> Check\n Check --> Work\n Work --> Done\n Done -->|retry| Start\n Work -->|again| Check\n"; + let text = render_text(code); + assert!(text.contains("again"), "{text}"); + assert!(text.contains("retry"), "{text}"); + assert_render( + code, r#" -graph LR - A[Start] --> B[End] - B --> A + ┌──────────────┐ + │ │ + ▼ │ + ┌───────┐ │ + │ Start │ │ + └───────┘ │ + │ │ + │ ┌─────┐ │ + │ │ │ │ + ▼ ▼ │ │ + ┌───────┐ │ │ + │ Check │ │ │ + └───────┘ │ │ + │ │ │ + │ │ again │ retry + │ │ │ + ▼ │ │ + ┌──────┐ │ │ + │ Work │ │ │ + └──────┘ │ │ + │ │ │ │ + │ └──────┘ │ + │ │ + ▼ │ + ┌──────┐ │ + │ Done │ │ + └──────┘ │ + │ │ + └───────────────┘ "#, ); + } - assert!(text.contains("Start")); - assert!(text.contains("End")); - assert!(text.contains('▶')); - assert!(text.contains('▲')); + #[test] + fn feedback_labels_sit_inline_on_their_lane_left_right() { + let code = "graph LR\n Start --> Check\n Check --> Work\n Work --> Done\n Done -->|retry| Start\n Work -->|again| Check\n"; + let text = render_text(code); + assert!(text.contains("again"), "{text}"); + assert!(text.contains("retry"), "{text}"); + assert_render( + code, + r#" + + ┌───────┐ ┌───────┐ ┌──────┐ ┌──────┐ + │ Start │─────▶│ Check │─────▶│ Work │─────▶│ Done │ + └───────┘ └───────┘ └──────┘ └──────┘ + ▲ ▲ └─┐ └─┐ +┌──┘ ┌──┘ │ │ +│ └─────────again──────────┘ │ +│ │ +└────────────────────────retry────────────────────────┘ +"#, + ); } } From 83ff40b3f6bca923035acac9017031434f1b5b61 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Fri, 4 Sep 2026 11:19:47 +0530 Subject: [PATCH 03/17] fix(diagram): budget rows and columns for feedback routes Review of the new feedback routing turned up back edges that merge into one ambiguous line, and back edges that vanish into a node box and reappear on the far side as if they had tunnelled through it. The reason is that the router picked its rows from a fixed table: two rows below the box for the source nearest the gutter, one for everybody else. That is fine for two sources and wrong for three. It also worked out to exactly the rows the entry side used, so a source in one layer and a target in the next fused their routes together whenever both landed in the same gap. Left-right had the same mistake in the other axis, measuring the drop and rise columns from the *node's* border -- but boxes are centred in a column as wide as its widest node, so a narrow box's own margin can sit squarely inside a taller neighbour. The route was drawn into the box, silently skipped, and what came out the other side read as an edge that does not exist. So stop guessing. Count the feedback endpoints per layer up front and size each gap to hold one row (or column) per endpoint on top of what the forward edges need. Nothing shares. Lanes measure from column edges, the only place guaranteed clear of every box. With no feedback edges the budget collapses to the old four-row gap and six-column gutter, so acyclic diagrams render byte for byte as they did before. Labels needed the same treatment. A straight forward edge writes its label on the first gap row, which is the row a feedback stem drops through, so the stem now leaves under the left border where nothing else writes. A left-right lane label that does not fit is cut with an ellipsis rather than painted over the route's own corner and off the edge of the canvas. And each gutter lane reserves the width of *its own* label instead of the longest one in the diagram, which was turning a single twenty-column label into twenty wasted columns per lane. Every route now asserts under test that it never lands on a node cell, and a seeded property test pushes two thousand assorted graphs through that check. Reverting any one of these fixes fails a test, which is rather the point. While at it, the render-with-timeout test helper reported a panicking render thread as a ten second timeout, because the sender drops on unwind and recv_timeout comes back Disconnected. A misleading error from a helper that only speaks up when something is already wrong. Please don't do that. --- src/diagram.rs | 854 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 657 insertions(+), 197 deletions(-) diff --git a/src/diagram.rs b/src/diagram.rs index 3c7e98a..ca21041 100644 --- a/src/diagram.rs +++ b/src/diagram.rs @@ -305,6 +305,9 @@ struct Layout { /// `feedback[k]`, so the shortest edge sits innermost and longer edges wrap /// around it instead of crossing it. feedback: Vec, + /// The same indices as a set, for the renderers' "is this edge routed as + /// feedback?" test. + feedback_set: HashSet, } fn layout(graph: &Graph) -> Layout { @@ -326,13 +329,14 @@ fn layout(graph: &Graph) -> Layout { _ => 0, } }; - let mut feedback: Vec = feedback_set.into_iter().collect(); + let mut feedback: Vec = feedback_set.iter().copied().collect(); feedback.sort_by_key(|&idx| (span(idx), idx)); Layout { layers, node_pos, feedback, + feedback_set, } } @@ -343,46 +347,66 @@ struct FeedbackPlan { /// Gutter lane, 0 = innermost. lane: usize, /// Rank of the source among the feedback sources in its layer, counted - /// from the gutter side (0 = nearest the gutter). Sources that share a - /// layer leave on different rows or columns so their routes do not merge. + /// from the gutter side (0 = nearest the gutter). Every rank gets its own + /// row (TD) or gap column (LR), so routes that share a layer neither merge + /// nor cross. src_rank: usize, /// Same for the destination among the feedback targets in its layer. dst_rank: usize, } -fn plan_feedback(graph: &Graph, layout: &Layout) -> Vec { - let mut sources: HashMap> = HashMap::new(); - let mut targets: HashMap> = HashMap::new(); +/// Routing decisions for all the feedback edges of one diagram. +struct FeedbackPlans { + plans: Vec, + /// Number of distinct feedback sources in each layer. The gap after a + /// layer has to hold one row (TD) or column (LR) per source. + exits: Vec, + /// Number of distinct feedback targets in each layer, likewise budgeted in + /// the gap before it. + entries: Vec, +} + +fn plan_feedback(graph: &Graph, layout: &Layout) -> FeedbackPlans { + let layer_count = layout.layers.len(); + let mut sources: Vec> = vec![BTreeSet::new(); layer_count]; + let mut targets: Vec> = vec![BTreeSet::new(); layer_count]; for &idx in &layout.feedback { let edge = &graph.edges[idx]; if let Some(&(layer, pos)) = layout.node_pos.get(&edge.from) { - sources.entry(layer).or_default().insert(pos); + sources[layer].insert(pos); } if let Some(&(layer, pos)) = layout.node_pos.get(&edge.to) { - targets.entry(layer).or_default().insert(pos); + targets[layer].insert(pos); } } - let rank = |set: &HashMap>, layer: usize, pos: usize| { - set.get(&layer) - .map_or(0, |positions| positions.range(pos + 1..).count()) - }; + // Rank counts the endpoints between this one and the gutter. + let rank = |positions: &BTreeSet, pos: usize| positions.range(pos + 1..).count(); - layout + let mut plans: Vec = layout .feedback .iter() - .enumerate() - .filter_map(|(lane, &idx)| { + .filter_map(|&idx| { let edge = &graph.edges[idx]; let &(src_layer, src_pos) = layout.node_pos.get(&edge.from)?; let &(dst_layer, dst_pos) = layout.node_pos.get(&edge.to)?; Some(FeedbackPlan { edge: idx, - lane, - src_rank: rank(&sources, src_layer, src_pos), - dst_rank: rank(&targets, dst_layer, dst_pos), + lane: 0, + src_rank: rank(&sources[src_layer], src_pos), + dst_rank: rank(&targets[dst_layer], dst_pos), }) }) - .collect() + .collect(); + // Lanes are numbered over the edges that survived, so they stay contiguous. + for (lane, plan) in plans.iter_mut().enumerate() { + plan.lane = lane; + } + + FeedbackPlans { + plans, + exits: sources.iter().map(BTreeSet::len).collect(), + entries: targets.iter().map(BTreeSet::len).collect(), + } } /// Geometry of one feedback edge route in a top-down diagram. @@ -411,13 +435,16 @@ pub(crate) struct FeedbackRouteLr { pub(crate) exit_x: usize, /// Row of the source's bottom border. pub(crate) src_bottom_y: usize, - /// Gap column right of the source that carries the drop to the lane. + /// Gap column right of the source's *column* that carries the drop to the + /// lane. Boxes are centred in a column sized by its widest node, so a + /// column edge is the only place guaranteed to be clear of every box. pub(crate) exit_lane_x: usize, /// Column where the arrowhead enters the destination's bottom border. pub(crate) entry_x: usize, /// Row of the destination's bottom border. pub(crate) dst_bottom_y: usize, - /// Gap column left of the destination that carries the rise from the lane. + /// Gap column left of the destination's column that carries the rise from + /// the lane. pub(crate) entry_lane_x: usize, /// Row of the horizontal lane in the gutter below the diagram. pub(crate) lane_y: usize, @@ -670,6 +697,26 @@ pub(crate) fn label_box_width(label: &str, shape: NodeShape) -> usize { width.max(7) } +/// Fit `label` into `width` columns, marking a cut with an ellipsis. +/// +/// Some routes have a fixed amount of room for their label (a left-right +/// lane runs between two node columns), and a label that overran it used to +/// paint over the route's own corner and off the edge of the canvas. +pub(crate) fn fit_label(label: &str, width: usize) -> String { + if label.chars().count() <= width { + return label.to_string(); + } + match width { + 0 => String::new(), + 1 => "\u{2026}".to_string(), + _ => label + .chars() + .take(width - 1) + .chain(std::iter::once('\u{2026}')) + .collect(), + } +} + // ───── Canvas ───── pub(crate) const CONN_UP: u8 = 1; @@ -762,6 +809,23 @@ impl Canvas { } } + /// Draw one cell of a feedback route. + /// + /// Feedback routes are planned to run only through gap rows and gap + /// columns, which never contain nodes. `add_connection` would silently + /// skip a node cell and leave a route that looks like it stops at a box, + /// so under test a violation of that invariant fails instead. + fn connect_route(&mut self, x: usize, y: usize, dir: u8, fg: Option) { + #[cfg(test)] + if y < self.height && x < self.width { + assert!( + !self.cells[y][x].is_node, + "feedback route runs through the node cell at ({x}, {y})" + ); + } + self.add_connection(x, y, dir, fg); + } + #[allow(clippy::too_many_arguments)] pub(crate) fn draw_node( &mut self, @@ -955,12 +1019,18 @@ impl Canvas { label: Option<&str>, edge_fg: Option, label_fg: Option, + bus_y_override: Option, ) { if src_bottom_y + 1 >= dst_top_y { return; } - let mid_y = src_bottom_y + 1 + (dst_top_y - src_bottom_y - 1) / 2; + // A bent edge draws its horizontal run on `mid_y` and its label on the + // row above. The caller overrides both when the gap also carries + // feedback routes, so that neither lands on a reserved row. + let mid_y = bus_y_override + .filter(|&y| y > src_bottom_y && y < dst_top_y) + .unwrap_or(src_bottom_y + 1 + (dst_top_y - src_bottom_y - 1) / 2); if src_cx == dst_cx { // Straight down @@ -1148,28 +1218,28 @@ impl Canvas { // Leave the source: stem down to the gap row, turn, run right to the lane. for y in (r.src_bottom_y + 1)..r.exit_y { - self.add_connection(r.exit_x, y, CONN_UP | CONN_DOWN, fg); + self.connect_route(r.exit_x, y, CONN_UP | CONN_DOWN, fg); } - self.add_connection(r.exit_x, r.exit_y, CONN_UP | CONN_RIGHT, fg); + self.connect_route(r.exit_x, r.exit_y, CONN_UP | CONN_RIGHT, fg); for x in (r.exit_x + 1)..r.lane_x { - self.add_connection(x, r.exit_y, CONN_LEFT | CONN_RIGHT, fg); + self.connect_route(x, r.exit_y, CONN_LEFT | CONN_RIGHT, fg); } - self.add_connection(r.lane_x, r.exit_y, CONN_LEFT | CONN_UP, fg); + self.connect_route(r.lane_x, r.exit_y, CONN_LEFT | CONN_UP, fg); // Up the lane. for y in (r.entry_y + 1)..r.exit_y { - self.add_connection(r.lane_x, y, CONN_UP | CONN_DOWN, fg); + self.connect_route(r.lane_x, y, CONN_UP | CONN_DOWN, fg); } // Back across the gap row above the destination, then down into it. - self.add_connection(r.lane_x, r.entry_y, CONN_DOWN | CONN_LEFT, fg); + self.connect_route(r.lane_x, r.entry_y, CONN_DOWN | CONN_LEFT, fg); for x in (r.entry_x + 1)..r.lane_x { - self.add_connection(x, r.entry_y, CONN_LEFT | CONN_RIGHT, fg); + self.connect_route(x, r.entry_y, CONN_LEFT | CONN_RIGHT, fg); } - self.add_connection(r.entry_x, r.entry_y, CONN_RIGHT | CONN_DOWN, fg); + self.connect_route(r.entry_x, r.entry_y, CONN_RIGHT | CONN_DOWN, fg); let arrow_y = r.dst_top_y.saturating_sub(1); for y in (r.entry_y + 1)..arrow_y { - self.add_connection(r.entry_x, y, CONN_UP | CONN_DOWN, fg); + self.connect_route(r.entry_x, y, CONN_UP | CONN_DOWN, fg); } self.set(r.entry_x, arrow_y, '▼', fg); } @@ -1190,39 +1260,41 @@ impl Canvas { /// └──────────────────────┘ /// ``` /// - /// Gap columns never contain nodes, so the route cannot pass through a - /// node stacked above or below either endpoint. + /// The drop and the rise use the gap columns beside the node's *column*, + /// not beside its box. Boxes are centred in a column as wide as its widest + /// node, so only a column edge is guaranteed clear of every box; a margin + /// measured from a narrow box can sit inside a wider neighbour. pub(crate) fn draw_feedback_edge_lr(&mut self, route: &FeedbackRouteLr, fg: Option) { let r = route; let exit_y = r.src_bottom_y + 1; let entry_y = r.dst_bottom_y + 2; // Leave the source: turn under its bottom border, run right, drop. - self.add_connection(r.exit_x, exit_y, CONN_UP | CONN_RIGHT, fg); + self.connect_route(r.exit_x, exit_y, CONN_UP | CONN_RIGHT, fg); for x in (r.exit_x + 1)..r.exit_lane_x { - self.add_connection(x, exit_y, CONN_LEFT | CONN_RIGHT, fg); + self.connect_route(x, exit_y, CONN_LEFT | CONN_RIGHT, fg); } - self.add_connection(r.exit_lane_x, exit_y, CONN_LEFT | CONN_DOWN, fg); + self.connect_route(r.exit_lane_x, exit_y, CONN_LEFT | CONN_DOWN, fg); for y in (exit_y + 1)..r.lane_y { - self.add_connection(r.exit_lane_x, y, CONN_UP | CONN_DOWN, fg); + self.connect_route(r.exit_lane_x, y, CONN_UP | CONN_DOWN, fg); } // Along the lane. - self.add_connection(r.exit_lane_x, r.lane_y, CONN_UP | CONN_LEFT, fg); + self.connect_route(r.exit_lane_x, r.lane_y, CONN_UP | CONN_LEFT, fg); for x in (r.entry_lane_x + 1)..r.exit_lane_x { - self.add_connection(x, r.lane_y, CONN_LEFT | CONN_RIGHT, fg); + self.connect_route(x, r.lane_y, CONN_LEFT | CONN_RIGHT, fg); } - self.add_connection(r.entry_lane_x, r.lane_y, CONN_UP | CONN_RIGHT, fg); + self.connect_route(r.entry_lane_x, r.lane_y, CONN_UP | CONN_RIGHT, fg); // Rise beside the destination, run right under it, then up into it. for y in (entry_y + 1)..r.lane_y { - self.add_connection(r.entry_lane_x, y, CONN_UP | CONN_DOWN, fg); + self.connect_route(r.entry_lane_x, y, CONN_UP | CONN_DOWN, fg); } - self.add_connection(r.entry_lane_x, entry_y, CONN_DOWN | CONN_RIGHT, fg); + self.connect_route(r.entry_lane_x, entry_y, CONN_DOWN | CONN_RIGHT, fg); for x in (r.entry_lane_x + 1)..r.entry_x { - self.add_connection(x, entry_y, CONN_LEFT | CONN_RIGHT, fg); + self.connect_route(x, entry_y, CONN_LEFT | CONN_RIGHT, fg); } - self.add_connection(r.entry_x, entry_y, CONN_LEFT | CONN_UP, fg); + self.connect_route(r.entry_x, entry_y, CONN_LEFT | CONN_UP, fg); self.set(r.entry_x, r.dst_bottom_y + 1, '▲', fg); } @@ -1273,7 +1345,8 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz if layers.is_empty() { return None; } - let plans = plan_feedback(graph, &layout); + let feedback = plan_feedback(graph, &layout); + let last_layer = layers.len() - 1; // Calculate node widths let mut widths: HashMap = HashMap::new(); @@ -1291,52 +1364,63 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz + layer.len().saturating_sub(1) * h_gap; max_layer_width = max_layer_width.max(w); } - let core_width = max_layer_width + 6; // margin on each side - let core_height = layers.len() * (node_height + edge_gap) - edge_gap; - let last_layer = layers.len() - 1; - // A feedback edge leaves its source below the box and enters its - // destination above the box. The source nearest the gutter turns on the - // second gap row, which keeps its route clear of a straight-edge label on - // the first; every other source turns on the first gap row and passes - // under its right-hand siblings. Entries mirror this above the destination. - // Two sources (or two targets) in one layer therefore never share a row. - let exit_rows_below = |src_rank: usize| if src_rank == 0 { 2 } else { 1 }; - let entry_rows_above = |dst_rank: usize| if dst_rank == 0 { 3 } else { 4 }; - - // Routes into the first layer or out of the last one need extra rows. - let mut top_margin = 0usize; - let mut bottom_margin = 0usize; - for plan in &plans { - let edge = &graph.edges[plan.edge]; - if let Some(&(layer, _)) = layout.node_pos.get(&edge.to) - && layer == 0 - { - top_margin = top_margin.max(entry_rows_above(plan.dst_rank)); - } - if let Some(&(layer, _)) = layout.node_pos.get(&edge.from) - && layer == last_layer - { - bottom_margin = bottom_margin.max(exit_rows_below(plan.src_rank)); - } - } - - let max_label_width = plans - .iter() - .filter_map(|plan| graph.edges[plan.edge].label.as_ref()) - .map(|label| label.chars().count()) - .max() - .unwrap_or(0); - // Lanes sit four columns apart, plus room for a label beside each lane. - let lane_gap = 4 + max_label_width; - let gutter_width = if plans.is_empty() { - 0 + // ── Row budget ── + // + // The gap under layer `k` holds, from the top: + // + // 1 row a straight forward edge's label, + // `exits[k]` rows one per feedback source in layer k, + // 2 rows a bent forward edge's label, then its bus, + // `entries[k+1]` rows one per feedback target in layer k+1, + // 1 row the forward arrowhead. + // + // So every feedback endpoint in the gap owns a row that no other route and + // no label writes to: two routes can neither merge into one ambiguous line + // nor be painted over. With no feedback edges this is the original + // four-row gap, and acyclic diagrams render exactly as they did before. + let gap_rows: Vec = (0..last_layer) + .map(|k| edge_gap + feedback.exits[k] + feedback.entries[k + 1]) + .collect(); + // A route into the first layer or out of the last one needs rows outside + // the diagram: one per rank, plus one for the arrowhead or the stem. + let margin = |count: usize| if count == 0 { 0 } else { count + 1 }; + let top_margin = margin(feedback.entries[0]); + let bottom_margin = margin(feedback.exits[last_layer]); + + // `gap_rows` has one entry per gap, so one fewer than there are layers. + let mut layer_top: Vec = Vec::with_capacity(layers.len()); + let mut next_y = top_margin; + for gap in &gap_rows { + layer_top.push(next_y); + next_y += node_height + gap; + } + layer_top.push(next_y); + let canvas_height = next_y + node_height + bottom_margin; + + // ── Gutter ── + // + // Lanes sit four columns apart, and a labelled lane additionally reserves + // the columns its own label occupies, so one long label no longer widens + // every other lane. + let label_width = |plan: &FeedbackPlan| { + graph.edges[plan.edge] + .label + .as_ref() + .map_or(0, |label| label.chars().count()) + }; + let mut lane_xs: Vec = Vec::with_capacity(feedback.plans.len()); + let mut gutter_x = core_width + 1; + for plan in &feedback.plans { + lane_xs.push(gutter_x); + gutter_x += 4 + label_width(plan); + } + let canvas_width = if feedback.plans.is_empty() { + core_width } else { - lane_gap * plans.len() + gutter_x }; - let canvas_width = core_width + gutter_width; - let canvas_height = top_margin + core_height + bottom_margin; let mut canvas = Canvas::new(canvas_width, canvas_height); @@ -1350,7 +1434,7 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz let canvas_center = core_width / 2; for (layer_idx, layer) in layers.iter().enumerate() { - let y = top_margin + layer_idx * (node_height + edge_gap); + let y = layer_top[layer_idx]; // Compute node centers relative to layer, then offset to center in canvas let node_widths_in_layer: Vec = layer @@ -1395,45 +1479,58 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz let edge_fg = Some(theme.code_border); let label_fg = Some(theme.h3); // Use a distinct color for edge labels - // Feedback edges go first so that forward-edge arrowheads and labels, - // which overwrite cells, end up on top of any route they touch. - let lane_x = |lane: usize| core_width + 1 + lane * lane_gap; - let mut labels: Vec<(usize, usize, &str)> = Vec::new(); - for plan in &plans { + // Feedback edges go first so that forward-edge arrowheads, which overwrite + // cells, end up on top of any route they cross. + let mut labels: Vec<(usize, usize, String)> = Vec::new(); + for plan in &feedback.plans { let edge = &graph.edges[plan.edge]; let (Some(src), Some(dst)) = (positions.get(&edge.from), positions.get(&edge.to)) else { continue; }; let route = FeedbackRouteTd { - exit_x: src.right_x().saturating_sub(1), + // Leave under the left border. A straight forward edge writes its + // label two columns right of the box centre on the first gap row, + // which is the row this stem drops through. + exit_x: src.left_x() + 1, src_bottom_y: src.bottom_y(), - exit_y: src.bottom_y() + exit_rows_below(plan.src_rank), + exit_y: src.bottom_y() + 2 + plan.src_rank, entry_x: dst.right_x().saturating_sub(1), dst_top_y: dst.top_y, - entry_y: dst.top_y.saturating_sub(entry_rows_above(plan.dst_rank)), - lane_x: lane_x(plan.lane), + entry_y: dst.top_y.saturating_sub(2 + plan.dst_rank), + lane_x: lane_xs[plan.lane], }; canvas.draw_feedback_edge_td(&route, edge_fg); if let Some(text) = edge.label.as_deref() { // Beside this edge's lane, level with the middle of its vertical run. let y = (route.entry_y + route.exit_y) / 2; - labels.push((route.lane_x + 2, y, text)); + labels.push((route.lane_x + 2, y, text.to_string())); } } - for (x, y, text) in labels { + for (x, y, text) in &labels { for (i, ch) in text.chars().enumerate() { - canvas.set(x + i, y, ch, label_fg); + canvas.set(x + i, *y, ch, label_fg); } } // Forward edges - let feedback_set: HashSet = layout.feedback.iter().copied().collect(); for (idx, edge) in graph.edges.iter().enumerate() { - if feedback_set.contains(&idx) { + if layout.feedback_set.contains(&idx) { continue; } if let (Some(src), Some(dst)) = (positions.get(&edge.from), positions.get(&edge.to)) { + // Between adjacent layers the bus sits below the gap's exit rows + // and above its entry rows, so neither it nor the label on the row + // above it can land on a feedback route. + let bus_y = match ( + layout.node_pos.get(&edge.from), + layout.node_pos.get(&edge.to), + ) { + (Some(&(from, _)), Some(&(to, _))) if to == from + 1 => { + Some(src.bottom_y() + 3 + feedback.exits[from]) + } + _ => None, + }; canvas.draw_edge_td( src.center_x, src.bottom_y(), @@ -1442,6 +1539,7 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz edge.label.as_deref(), edge_fg, label_fg, + bus_y, ); } } @@ -1463,7 +1561,8 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz if layers.is_empty() { return None; } - let plans = plan_feedback(graph, &layout); + let feedback = plan_feedback(graph, &layout); + let last_layer = layers.len() - 1; // Calculate node widths let mut widths: HashMap = HashMap::new(); @@ -1485,14 +1584,37 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz let max_nodes_in_layer = layers.iter().map(|l| l.len()).max().unwrap_or(1); - let canvas_width: usize = - col_widths.iter().sum::() + (layers.len().saturating_sub(1)) * node_h_gap + 4; + // ── Column budget ── + // + // The gap right of column `k` carries the drop column of every feedback + // source in column k and the rise column of every feedback target in + // column k+1, and still has to leave the forward edges' bend column clear + // between them. With no feedback edges the gap is the original six + // columns, so acyclic diagrams are unaffected. + let gap_widths: Vec = (0..last_layer) + .map(|k| { + let exits = feedback.exits[k]; + let entries = feedback.entries[k + 1]; + node_h_gap + .max(exits + entries + 4) + .max(2 * exits) + .max(2 * entries + 2) + }) + .collect(); + // Routes into the first column or out of the last one use the margins. + let left_margin = 2.max(feedback.entries[0]); + let right_margin = 2.max(feedback.exits[last_layer]); + + let canvas_width: usize = left_margin + + col_widths.iter().sum::() + + gap_widths.iter().sum::() + + right_margin; let core_height = max_nodes_in_layer * (node_height + v_gap) - v_gap + 2; // One spare row under the diagram, then a lane every `lane_gap` rows. - let gutter_height = if plans.is_empty() { + let gutter_height = if feedback.plans.is_empty() { 0 } else { - lane_gap * plans.len() + 1 + lane_gap * feedback.plans.len() + 1 }; let canvas_height = core_height + gutter_height; @@ -1504,7 +1626,7 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz // (left, right) border columns of each layer's column. let mut col_bounds: Vec<(usize, usize)> = Vec::with_capacity(layers.len()); - let mut col_x = 2; // starting x with margin + let mut col_x = left_margin; for (layer_idx, layer) in layers.iter().enumerate() { let col_w = col_widths[layer_idx]; col_bounds.push((col_x, col_x + col_w - 1)); @@ -1531,53 +1653,61 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz ); } - col_x += col_w + node_h_gap; + col_x += col_w + gap_widths.get(layer_idx).copied().unwrap_or(0); } let edge_fg = Some(theme.code_border); let label_fg = Some(theme.h3); - // Feedback edges go first so that forward-edge arrowheads and labels, - // which overwrite cells, end up on top of any route they touch. + // Feedback edges go first so that forward-edge arrowheads, which overwrite + // cells, end up on top of any route they cross. let lane_y = |lane: usize| core_height + 1 + lane * lane_gap; - let mut labels: Vec<(usize, usize, &str)> = Vec::new(); - for plan in &plans { + let mut labels: Vec<(usize, usize, String)> = Vec::new(); + for plan in &feedback.plans { let edge = &graph.edges[plan.edge]; let (Some(src), Some(dst)) = (positions.get(&edge.from), positions.get(&edge.to)) else { continue; }; - // The source nearest the gutter drops one column clear of its border - // and any other source two columns, so two sources stacked in one - // column use different gap columns and an upper source's drop does - // not hug the box below it. + let (Some(&(src_layer, _)), Some(&(dst_layer, _))) = ( + layout.node_pos.get(&edge.from), + layout.node_pos.get(&edge.to), + ) else { + continue; + }; + // Drop and rise beside the node's column rather than beside its box: a + // narrow box's own margin can sit inside a wider box stacked with it, + // and a route through a box is drawn as if it were not there at all. + // One column per rank keeps stacked endpoints on separate routes. let route = FeedbackRouteLr { exit_x: src.right_x().saturating_sub(1), src_bottom_y: src.bottom_y(), - exit_lane_x: src.right_x() + if plan.src_rank == 0 { 1 } else { 2 }, + exit_lane_x: col_bounds[src_layer].1 + 1 + plan.src_rank, entry_x: dst.left_x() + 1, dst_bottom_y: dst.bottom_y(), - entry_lane_x: dst.left_x().saturating_sub(2), + entry_lane_x: col_bounds[dst_layer].0.saturating_sub(1 + plan.dst_rank), lane_y: lane_y(plan.lane), }; canvas.draw_feedback_edge_lr(&route, edge_fg); if let Some(text) = edge.label.as_deref() { - // Inline on the lane, centered on its horizontal run. + // Inline on the lane, centered on its horizontal run. The run is + // all the room there is, so a longer label is cut rather than + // written over the route's own corner. let inner = route.exit_lane_x.saturating_sub(route.entry_lane_x + 1); + let text = fit_label(text, inner); let x = route.entry_lane_x + 1 + inner.saturating_sub(text.chars().count()) / 2; labels.push((x, route.lane_y, text)); } } - for (x, y, text) in labels { + for (x, y, text) in &labels { for (i, ch) in text.chars().enumerate() { - canvas.set(x + i, y, ch, label_fg); + canvas.set(x + i, *y, ch, label_fg); } } // Forward edges - let feedback_set: HashSet = layout.feedback.iter().copied().collect(); for (idx, edge) in graph.edges.iter().enumerate() { - if feedback_set.contains(&idx) { + if layout.feedback_set.contains(&idx) { continue; } if let (Some(src), Some(dst)) = (positions.get(&edge.from), positions.get(&edge.to)) { @@ -1655,12 +1785,23 @@ mod tests { thread::spawn(move || { let _ = tx.send(render_text(code)); }); - rx.recv_timeout(Duration::from_secs(10)) - .expect("rendering did not finish within 10 seconds") + match rx.recv_timeout(Duration::from_secs(10)) { + Ok(text) => text, + // The sender is dropped as soon as the render thread unwinds, so a + // panic there arrives here as a disconnect, not as a timeout. + Err(mpsc::RecvTimeoutError::Disconnected) => panic!("the render thread panicked"), + Err(mpsc::RecvTimeoutError::Timeout) => { + panic!("rendering did not finish within 10 seconds") + } + } } /// Compare a render against a snapshot written at column 0 inside a raw /// string literal (one leading newline, trailing blank rows ignored). + /// + /// Every render also checks an invariant of its own: [`Canvas::connect_route`] + /// fails the test if a feedback route is laid over a node cell, which is + /// what a route that runs through a box looks like. fn assert_render(code: &'static str, expected: &str) { let text = render_text_with_timeout(code); let expected = expected.strip_prefix('\n').unwrap_or(expected); @@ -1709,6 +1850,54 @@ mod tests { } } + /// Render a deterministic spread of graph shapes and let + /// [`Canvas::connect_route`] check every feedback route against every box. + /// + /// The shapes vary in node count, node width, edge count, direction and + /// labelling, because a route only collides with a box that a *differently + /// sized* neighbour widened the column for. + #[test] + fn feedback_routes_never_run_through_a_node_box() { + // xorshift with a fixed seed, so a failure is always reproducible. + let mut state: u64 = 0x2545_F491_4F6C_DD1D; + let mut next = move || { + state ^= state << 13; + state ^= state >> 7; + state ^= state << 17; + state + }; + for case in 0..2000u32 { + let n = 2 + (next() % 14) as usize; + let lr = next() % 2 == 0; + let mut code = String::from(if lr { "graph LR\n" } else { "graph TD\n" }); + let names: Vec = (0..n) + .map(|i| format!("N{i}[{}]", "x".repeat(1 + (i % 4) * 5))) + .collect(); + for _ in 0..1 + (next() % 30) { + let a = (next() % n as u64) as usize; + let b = (next() % n as u64) as usize; + if next() % 3 == 0 { + let label = "L".repeat(1 + (next() % 12) as usize); + code.push_str(&format!(" {} -->|{label}| {}\n", names[a], names[b])); + } else { + code.push_str(&format!(" {} --> {}\n", names[a], names[b])); + } + } + + let theme = Theme::dark(); + let leaked: &'static str = Box::leak(code.clone().into_boxed_str()); + let (tx, rx) = mpsc::channel(); + let handle = thread::spawn(move || { + let _ = tx.send(render_mermaid(leaked, &theme).is_some()); + }); + if let Err(err) = rx.recv_timeout(Duration::from_secs(20)) { + let _ = handle.join(); + panic!("case {case} failed ({err:?}) for:\n{code}"); + } + handle.join().unwrap(); + } + } + // ── Cycle classification ── #[test] @@ -1740,6 +1929,21 @@ mod tests { assert_eq!(layout.feedback, vec![4, 3]); } + #[test] + fn feedback_endpoints_are_counted_per_layer() { + let graph = parse_mermaid("graph TD\n S --> A\n S --> B\n A --> S\n B --> S\n") + .unwrap(); + let layout = layout(&graph); + let feedback = plan_feedback(&graph, &layout); + // Both back edges leave the second layer and enter the first. + assert_eq!(feedback.exits, vec![0, 2]); + assert_eq!(feedback.entries, vec![1, 0]); + // Two sources in one layer get distinct ranks. + let mut ranks: Vec = feedback.plans.iter().map(|p| p.src_rank).collect(); + ranks.sort_unstable(); + assert_eq!(ranks, vec![0, 1]); + } + // ── Acyclic rendering must not change ── #[test] @@ -1775,6 +1979,25 @@ mod tests { ); } + #[test] + fn acyclic_left_right_diagram() { + assert_render( + "graph LR\n A[Start] --> B{Decision}\n B -->|Yes| C[Action 1]\n B -->|No| D[Do]\n C --> E[End]\n D --> E\n", + r#" + + ┌──────────┐ + ┌─YesAction 1 │───┐ + ┌───────┐ ◆────────────◆ │ └──────────┘ │ ┌─────┐ + │ Start │─────▶│ Decision │───┤ ├─▶│ End │ + └───────┘ ◆────────────◆ │ No │ └─────┘ + │ ┌─────┐ │ + └────▶│ Do │─────┘ + └─────┘ + +"#, + ); + } + // ── Cycles ── #[test] @@ -1783,7 +2006,6 @@ mod tests { "graph TB\n Loop --> Execute\n Execute --> Repeat\n Repeat --> Loop\n", r#" ┌───────┐ - │ │ ▼ │ ┌──────┐ │ │ Loop │ │ @@ -1802,8 +2024,8 @@ mod tests { ┌────────┐ │ │ Repeat │ │ └────────┘ │ - │ │ - └──────┘ + │ │ + └─────────────┘ "#, ); } @@ -1818,8 +2040,9 @@ mod tests { │ Start │─────▶│ End │ └───────┘ └─────┘ ▲ └─┐ -┌──┘ │ -└───────────────────────┘ + ┌─┘ │ + └──────────────────────┘ + "#, ); } @@ -1830,13 +2053,12 @@ mod tests { "graph TD\n A[Self] --> A\n", r#" ┌─────┐ - │ │ ▼ │ ┌──────┐ │ │ Self │ │ └──────┘ │ - │ │ - └─────┘ + │ │ + └──────────┘ "#, ); } @@ -1851,8 +2073,9 @@ mod tests { │ Self │ └──────┘ ▲ └─┐ -┌──┘ │ -└─────────┘ + ┌─┘ │ + └────────┘ + "#, ); } @@ -1861,19 +2084,18 @@ mod tests { #[test] fn feedback_edge_avoids_sibling_nodes_top_down() { - let code = "graph TD\n A --> B\n A --> C\n B --> D\n C --> D\n D --> B\n"; - let text = render_text(code); - // Nothing may be drawn between the two siblings on their middle row. - assert!(text.contains("│ B │ │ C │"), "{text}"); + // The back edge into B leaves D, wraps around the gutter and comes back + // through a gap row. It never touches the row C is drawn on. assert_render( - code, + "graph TD\n A --> B\n A --> C\n B --> D\n C --> D\n D --> B\n", r#" ┌─────┐ │ A │ └─────┘ │ - ┌───┼────────────┐ - ┌─┼───┴────┐ │ + │ + ┌─────┴────┐ + │ ┌────────┼───────┐ ▼ ▼ ▼ │ ┌─────┐ ┌─────┐ │ │ B │ │ C │ │ @@ -1885,43 +2107,83 @@ mod tests { ┌─────┐ │ │ D │ │ └─────┘ │ - │ │ - └──────────┘ + │ │ + └──────────────┘ "#, ); } #[test] fn feedback_edge_avoids_stacked_nodes_left_right() { - let code = "graph LR\n A --> B\n A --> C\n B --> D\n C --> D\n D --> B\n"; - let text = render_text(code); - // C sits directly below B; the route into B must not cross C's box. - let c_col = text - .lines() - .find_map(|line| line.find("│ C │")) - .expect("C is drawn"); - for line in text.lines() { - let cell = line.chars().nth(c_col + 3).unwrap_or(' '); - assert!( - cell != '│' || line.contains("│ B │") || line.contains("│ C │"), - "line through the column of C:\n{text}" - ); - } + // C sits directly below B; the route into B rises beside their column. assert_render( - code, + "graph LR\n A --> B\n A --> C\n B --> D\n C --> D\n D --> B\n", r#" ┌─────┐ ┌─▶│ B │───┐ ┌─────┐ │ └─────┘ │ ┌─────┐ │ A │───┤ ▲ ├─▶│ D │ - └─────┘ │┌──┘ │ └─────┘ - ││ ┌─────┐ │ └─┐ - └┼▶│ C │───┘ │ - │ └─────┘ │ - │ │ - │ │ - └─────────────────────┘ + └─────┘ │ ┌─┘ │ └─────┘ + │ │┌─────┐ │ └─┐ + └─▶│ C │───┘ │ + │└─────┘ │ + │ │ + │ │ + └────────────────────┘ + +"#, + ); + } + + #[test] + fn feedback_route_clears_a_wider_stacked_node_left_right() { + // B is narrower than the node stacked under it. Its own right margin is + // inside that wider box, so the route has to use the column edge; drawing + // it over the box would leave a line that stops dead at the border. + assert_render( + "graph LR\n A --> B\n A --> C[VeryLongName]\n B --> D\n C --> D\n D --> B\n", + r#" + + ┌─────┐ + ┌──────▶│ B │───────┐ + ┌─────┐ │ └─────┘ │ ┌─────┐ + │ A │───┤ ▲ ├─▶│ D │ + └─────┘ │ ┌──────┘ │ └─────┘ + │ │┌──────────────┐ │ └─┐ + └─▶│ VeryLongName │───┘ │ + │└──────────────┘ │ + │ │ + │ │ + └─────────────────────────────┘ + +"#, + ); + } + + // ── Endpoints that share a layer, and gaps that carry both ── + + #[test] + fn feedback_route_clears_a_wider_stacked_source_left_right() { + // The mirror of the case above: B leaves through a column edge because + // its own right margin is inside the wider box stacked under it. The + // crossing with C -> D is a junction, not a break. + assert_render( + "graph LR\n A --> B\n A --> C[VeryLongName]\n B --> D\n C --> D\n B --> A\n", + r#" + + ┌─────┐ + ┌──────▶│ B │───────┐ + ┌─────┐ │ └─────┘ │ ┌─────┐ + │ A │───┤ └─────┐ ├─▶│ D │ + └─────┘ │ │ │ └─────┘ + ▲ │ ┌──────────────┐│ │ + ┌─┘ └─▶│ VeryLongName │┼──┘ + │ └──────────────┘│ + │ │ + │ │ + └─────────────────────────────┘ + "#, ); } @@ -1932,7 +2194,6 @@ mod tests { "graph TD\n S --> A\n S --> B\n A --> S\n B --> S\n", r#" ┌──────────┬───┐ - │ │ │ ▼ │ │ ┌─────┐ │ │ │ S │ │ │ @@ -1944,8 +2205,36 @@ mod tests { ┌─────┐ ┌─────┐ │ │ │ A │ │ B │ │ │ └─────┘ └─────┘ │ │ - └──────────┼─────┘ │ - └─────────┘ + │ │ │ │ + │ └─────────┼───┘ + └────────────────────┘ +"#, + ); + } + + #[test] + fn three_feedback_sources_in_one_layer_leave_on_different_rows() { + // A third source needs a third row: one row per rank, not one row for the + // nearest source and one shared by all the rest. + assert_render( + "graph TD\n S --> A\n S --> B\n S --> C\n A --> S\n B --> S\n C --> S\n", + r#" + ┌────────────────┬───┬───┐ + ▼ │ │ │ + ┌─────┐ │ │ │ + │ S │ │ │ │ + └─────┘ │ │ │ + │ │ │ │ + │ │ │ │ + ┌──────────┼──────────┐ │ │ │ + ▼ ▼ ▼ │ │ │ + ┌─────┐ ┌─────┐ ┌─────┐ │ │ │ + │ A │ │ B │ │ C │ │ │ │ + └─────┘ └─────┘ └─────┘ │ │ │ + │ │ │ │ │ │ + │ │ └─────────┼───┼───┘ + │ └────────────────────┼───┘ + └───────────────────────────────┘ "#, ); } @@ -1962,13 +2251,56 @@ mod tests { │ S │───┤ └──┐ └─────┘ │ │ ▲ │ ┌─────┐ │ -┌──┘ └─▶│ B │ │ -│ └─────┘ │ -│ └─┐│ -│ ││ -├─────────────────────┼┘ -│ │ -└─────────────────────┘ + ┌─┘ └─▶│ B │ │ + │ └─────┘ │ + │ └─┐│ + │ ││ + ├────────────────────┼┘ + │ │ + └────────────────────┘ + +"#, + ); + } + + #[test] + fn feedback_exit_and_entry_in_one_gap_use_different_rows() { + // B is a feedback source and C, one layer below it, is a feedback target, + // so one gap carries both. Sharing a row would fuse the two routes into a + // single line that reads as neither. + assert_render( + "graph TD\n A --> B\n B --> C\n C --> D\n D --> C\n B --> A\n", + r#" + ┌─────────┐ + ▼ │ + ┌─────┐ │ + │ A │ │ + └─────┘ │ + │ │ + │ │ + │ │ + ▼ │ + ┌─────┐ │ + │ B │ │ + └─────┘ │ + │ │ │ + └─┼───────────┘ + │ + │ + │ ┌─────┐ + ▼ ▼ │ + ┌─────┐ │ + │ C │ │ + └─────┘ │ + │ │ + │ │ + │ │ + ▼ │ + ┌─────┐ │ + │ D │ │ + └─────┘ │ + │ │ + └─────────┘ "#, ); } @@ -1977,22 +2309,18 @@ mod tests { #[test] fn feedback_labels_sit_beside_their_own_lane_top_down() { - let code = "graph TD\n Start --> Check\n Check --> Work\n Work --> Done\n Done -->|retry| Start\n Work -->|again| Check\n"; - let text = render_text(code); - assert!(text.contains("again"), "{text}"); - assert!(text.contains("retry"), "{text}"); assert_render( - code, + "graph TD\n Start --> Check\n Check --> Work\n Work --> Done\n Done -->|retry| Start\n Work -->|again| Check\n", r#" ┌──────────────┐ - │ │ ▼ │ ┌───────┐ │ │ Start │ │ └───────┘ │ + │ │ + │ │ │ │ │ ┌─────┐ │ - │ │ │ │ ▼ ▼ │ │ ┌───────┐ │ │ │ Check │ │ │ @@ -2004,37 +2332,169 @@ mod tests { ┌──────┐ │ │ │ Work │ │ │ └──────┘ │ │ - │ │ │ │ - │ └──────┘ │ + │ │ │ │ + └──┼────────┘ │ + │ │ │ │ ▼ │ ┌──────┐ │ │ Done │ │ └──────┘ │ - │ │ - └───────────────┘ + │ │ + └────────────────────┘ "#, ); } #[test] fn feedback_labels_sit_inline_on_their_lane_left_right() { - let code = "graph LR\n Start --> Check\n Check --> Work\n Work --> Done\n Done -->|retry| Start\n Work -->|again| Check\n"; - let text = render_text(code); - assert!(text.contains("again"), "{text}"); - assert!(text.contains("retry"), "{text}"); assert_render( - code, + "graph LR\n Start --> Check\n Check --> Work\n Work --> Done\n Done -->|retry| Start\n Work -->|again| Check\n", r#" ┌───────┐ ┌───────┐ ┌──────┐ ┌──────┐ │ Start │─────▶│ Check │─────▶│ Work │─────▶│ Done │ └───────┘ └───────┘ └──────┘ └──────┘ ▲ ▲ └─┐ └─┐ -┌──┘ ┌──┘ │ │ -│ └─────────again──────────┘ │ -│ │ -└────────────────────────retry────────────────────────┘ + ┌─┘ ┌─┘ │ │ + │ └─────────again─────────┘ │ + │ │ + └───────────────────────retry────────────────────────┘ + +"#, + ); + } + + #[test] + fn a_label_longer_than_its_lane_is_truncated() { + // The lane runs between two node columns and that is all the room there + // is, so the label is cut rather than written over the corner and off + // the edge of the canvas. + assert_render( + "graph LR\n A -->|this is a long label| A\n", + r#" + + ┌─────┐ + │ A │ + └─────┘ + ▲ └─┐ + ┌─┘ │ + └this i…┘ + +"#, + ); + } + + #[test] + fn straight_forward_edge_label_clears_the_feedback_stem() { + // The label on B -> C sits on the first gap row, which is the row the + // feedback stem drops through. The stem leaves under the left border. + assert_render( + "graph TD\n A --> B\n B -->|ok| C\n B --> A\n", + r#" + ┌─────┐ + ▼ │ + ┌─────┐ │ + │ A │ │ + └─────┘ │ + │ │ + │ │ + │ │ + ▼ │ + ┌─────┐ │ + │ B │ │ + └─────┘ │ + │ │ ok │ + └─┼───────┘ + │ + │ + ▼ + ┌─────┐ + │ C │ + └─────┘ +"#, + ); + } + + #[test] + fn bent_forward_edge_label_clears_the_feedback_route() { + // The yes/no labels sit on the row above the bus, which the row budget + // keeps below every exit row in the gap. + assert_render( + "graph TD\n A --> B\n B -->|yes| C\n B -->|no| D\n C --> E\n D --> E\n B --> A\n", + r#" + ┌──────────┐ + ▼ │ + ┌─────┐ │ + │ A │ │ + └─────┘ │ + │ │ + │ │ + │ │ + ▼ │ + ┌─────┐ │ + │ B │ │ + └─────┘ │ + │ │ │ + └─┼────────────┘ + yes │no + ┌─────┴────┐ + ▼ ▼ + ┌─────┐ ┌─────┐ + │ C │ │ D │ + └─────┘ └─────┘ + │ │ + │ │ + └─────┬────┘ + ▼ + ┌─────┐ + │ E │ + └─────┘ +"#, + ); + } + + #[test] + fn one_long_label_does_not_widen_every_lane() { + let code = "graph TD\n Start --> Check\n Check --> Work\n Work --> Done\n Done -->|x| Start\n Work -->|a very long label| Check\n"; + let text = render_text(code); + let width = text.lines().map(|l| l.chars().count()).max().unwrap_or(0); + // Each lane reserves its own label's width. Charging every lane for the + // longest label would add another 16 columns here. + assert!(width < 45, "gutter is {width} columns wide:\n{text}"); + assert_render( + code, + r#" + ┌──────────────────────────┐ + ▼ │ + ┌───────┐ │ + │ Start │ │ + └───────┘ │ + │ │ + │ │ + │ │ + │ ┌─────┐ │ + ▼ ▼ │ │ + ┌───────┐ │ │ + │ Check │ │ │ + └───────┘ │ │ + │ │ │ + │ │ a very long label │ x + │ │ │ + ▼ │ │ + ┌──────┐ │ │ + │ Work │ │ │ + └──────┘ │ │ + │ │ │ │ + └──┼────────┘ │ + │ │ + │ │ + ▼ │ + ┌──────┐ │ + │ Done │ │ + └──────┘ │ + │ │ + └────────────────────────────────┘ "#, ); } From daf30c7b51ae3c2c5c7d203fb230b1d7db846350 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Fri, 4 Sep 2026 13:11:30 +0530 Subject: [PATCH 04/17] fix(diagram): keep labels and arrowheads off feedback routes The previous commit budgeted a row or a column for every feedback endpoint so that no two routes could merge. Then it drew all the labels on top of them. Labels go on last and overwrite whatever is underneath, so a label that lands on a route hides the route rather than the other way round. Four did. A top-down gutter label took the middle row of its own lane, which is a gap row an outer lane runs along. A left-right bent label started two columns right of the bend, which is where the route into the next column rises. A straight one started two columns right of its box, which is the column a stacked sibling drops through. A lane label centred itself across its whole run, crossings included. None of this looks like corruption. You get a route with a word in the middle of it, which reads as an edge that stops there. Labels now take only the columns the routes left free, cut with an ellipsis when there are not enough of them, and a lane label takes the longest unbroken stretch of its own run rather than the middle. Two other things drawn after the routes could cover them. A forward arrowhead lands in the column immediately left of its box, which was also the rank-0 rise column, so rises now start one column further out. And a forward edge spanning more than one layer picks the midpoint of its own span, which no gap budget covers, so it is nudged off any row or column a route reserved. While at it: drop the unused `_src_cx` parameter from draw_edge_lr, and stop the property test leaking a string per case and passing silently when a render returns None. Top-down acyclic renders are still byte-identical to main. Left-right acyclic changes once more beyond the arrow fix: a bent label no longer overwrites the destination box, which the previous commit listed as not addressed. Every render now asserts that no label lands on a feedback route, and reverting any one of the fixes above fails at least one test. Claude-Session: https://claude.ai/code/session_01YSfTy6Fz4tGed1Fpe8ESe7 --- src/diagram.rs | 555 +++++++++++++++++++++++++++++++++++++++++-------- src/json.rs | 2 +- 2 files changed, 470 insertions(+), 87 deletions(-) diff --git a/src/diagram.rs b/src/diagram.rs index ca21041..3140349 100644 --- a/src/diagram.rs +++ b/src/diagram.rs @@ -444,7 +444,9 @@ pub(crate) struct FeedbackRouteLr { /// Row of the destination's bottom border. pub(crate) dst_bottom_y: usize, /// Gap column left of the destination's column that carries the rise from - /// the lane. + /// the lane, two columns clear of it rather than one: the column + /// immediately left of a box is where every forward arrowhead into it + /// lands, and sharing it would overwrite the junction. pub(crate) entry_lane_x: usize, /// Row of the horizontal lane in the gutter below the diagram. pub(crate) lane_y: usize, @@ -752,6 +754,9 @@ pub(crate) struct CanvasCell { pub(crate) bg: Option, pub(crate) is_node: bool, pub(crate) connects: u8, + /// Drawn by a feedback route. Labels go on last and overwrite whatever is + /// under them, so this is what [`Canvas::set_label`] checks against. + is_feedback: bool, } impl Default for CanvasCell { @@ -762,6 +767,7 @@ impl Default for CanvasCell { bg: None, is_node: false, connects: 0, + is_feedback: false, } } } @@ -824,6 +830,68 @@ impl Canvas { ); } self.add_connection(x, y, dir, fg); + if y < self.height && x < self.width { + self.cells[y][x].is_feedback = true; + } + } + + /// Write one character of a label. + /// + /// Labels are drawn after the routes and overwrite whatever is under them, + /// so one placed badly hides a route instead of the other way round. Under + /// test the only cells a label may take are ones no feedback route drew, + /// plus, for a label riding inline on a left-right lane, that lane's own + /// plain horizontal run. + fn set_label(&mut self, x: usize, y: usize, ch: char, fg: Option, on_lane: bool) { + // Only the assertion below reads `on_lane`, and it is test-only. + #[cfg(not(test))] + let _ = on_lane; + #[cfg(test)] + if y < self.height && x < self.width { + let cell = &self.cells[y][x]; + let allowed = !cell.is_feedback + || (on_lane && !cell.is_node && cell.connects == (CONN_LEFT | CONN_RIGHT)); + assert!( + allowed, + "label character {ch:?} lands on the feedback route cell at ({x}, {y})" + ); + } + self.set(x, y, ch, fg); + } + + /// True when the cell carries a plain horizontal run and nothing else, so + /// a label may be written over it without hiding a corner, a crossing or + /// a box. + fn is_plain_horizontal(&self, x: usize, y: usize) -> bool { + y < self.height && x < self.width && { + let cell = &self.cells[y][x]; + !cell.is_node && cell.connects == (CONN_LEFT | CONN_RIGHT) + } + } + + /// Longest stretch of plain horizontal run in `lo..=hi` on row `y`, + /// as `(start, length)`. Length is zero when there is no such stretch. + fn longest_plain_run(&self, y: usize, lo: usize, hi: usize) -> (usize, usize) { + if hi < lo { + return (lo, 0); + } + let (mut best_start, mut best_len) = (lo, 0); + let (mut start, mut len) = (lo, 0); + for x in lo..=hi { + if !self.is_plain_horizontal(x, y) { + len = 0; + continue; + } + if len == 0 { + start = x; + } + len += 1; + if len > best_len { + best_len = len; + best_start = start; + } + } + (best_start, best_len) } #[allow(clippy::too_many_arguments)] @@ -1009,6 +1077,9 @@ impl Canvas { row_ys } + /// `label_max_x` is the last column a label may use. The gutter lanes sit + /// right of the diagram on every row, so a label long enough to reach them + /// would cut a feedback route; it is truncated instead. #[allow(clippy::too_many_arguments)] pub(crate) fn draw_edge_td( &mut self, @@ -1020,6 +1091,7 @@ impl Canvas { edge_fg: Option, label_fg: Option, bus_y_override: Option, + label_max_x: Option, ) { if src_bottom_y + 1 >= dst_top_y { return; @@ -1043,8 +1115,10 @@ impl Canvas { // Place label beside the vertical line if let Some(text) = label { let label_y = src_bottom_y + 1; - for (i, ch) in text.chars().enumerate() { - self.set(src_cx + 2 + i, label_y, ch, label_fg); + let label_x = src_cx + 2; + let room = label_max_x.map_or(usize::MAX, |hi| (hi + 1).saturating_sub(label_x)); + for (i, ch) in fit_label(text, room).chars().enumerate() { + self.set_label(label_x + i, label_y, ch, label_fg, false); } } } else { @@ -1092,20 +1166,27 @@ impl Canvas { let label_len = text.chars().count(); let label_start = min_x + (max_x - min_x).saturating_sub(label_len) / 2; let label_y = if mid_y > 0 { mid_y - 1 } else { mid_y }; - for (i, ch) in text.chars().enumerate() { + let room = + label_max_x.map_or(usize::MAX, |hi| (hi + 1).saturating_sub(label_start)); + for (i, ch) in fit_label(text, room).chars().enumerate() { let lx = label_start + i; if lx < self.width { - self.set(lx, label_y, ch, label_fg); + self.set_label(lx, label_y, ch, label_fg, false); } } } } } + /// `label_bounds` is the inclusive range of columns a label may occupy. + /// The gap between two node columns also carries the drop and rise + /// columns of the feedback routes, and a label written over one of those + /// hides the route instead of the other way round, so the renderer passes + /// the columns that are free and a label too long for them is cut. + /// `None` leaves the label unbounded. #[allow(clippy::too_many_arguments)] pub(crate) fn draw_edge_lr( &mut self, - _src_cx: usize, src_right_x: usize, src_cy: usize, dst_left_x: usize, @@ -1114,6 +1195,7 @@ impl Canvas { edge_fg: Option, label_fg: Option, mid_x_override: Option, + label_bounds: Option<(usize, usize)>, ) { if src_right_x + 1 >= dst_left_x { return; @@ -1130,12 +1212,18 @@ impl Canvas { // Arrow replaces last segment self.set(dst_left_x - 1, dst_cy, '▶', edge_fg); - // Label above the horizontal line + // Label above the horizontal line, held inside the free columns. if let Some(text) = label { - let label_x = src_right_x + 2; let label_y = if src_cy > 0 { src_cy - 1 } else { 0 }; - for (i, ch) in text.chars().enumerate() { - self.set(label_x + i, label_y, ch, label_fg); + let (label_x, room) = match label_bounds { + Some((lo, hi)) => { + let x = (src_right_x + 2).max(lo); + (x, (hi + 1).saturating_sub(x)) + } + None => (src_right_x + 2, usize::MAX), + }; + for (i, ch) in fit_label(text, room).chars().enumerate() { + self.set_label(label_x + i, label_y, ch, label_fg, false); } } } else { @@ -1178,11 +1266,33 @@ impl Canvas { // Arrow self.set(dst_left_x - 1, dst_cy, '▶', edge_fg); - // Label near the vertical segment + // Label beside the vertical segment: right of the bend where + // there is room for it, otherwise left of the bend, and cut when + // neither side can hold it. Running past the free columns would + // hide a feedback route or the destination box. if let Some(text) = label { let label_y = min_y + (max_y - min_y).saturating_sub(1) / 2; - for (i, ch) in text.chars().enumerate() { - self.set(mid_x + 2 + i, label_y, ch, label_fg); + let len = text.chars().count(); + let (label_x, room) = match label_bounds { + Some((lo, hi)) => { + let right_x = (mid_x + 2).max(lo); + let right_room = (hi + 1).saturating_sub(right_x); + let left_hi = mid_x.saturating_sub(1).min(hi); + let left_room = (left_hi + 1).saturating_sub(lo); + if len <= right_room { + (right_x, right_room) + } else if len <= left_room { + (left_hi + 1 - len, left_room) + } else if right_room >= left_room { + (right_x, right_room) + } else { + (lo, left_room) + } + } + None => (mid_x + 2, usize::MAX), + }; + for (i, ch) in fit_label(text, room).chars().enumerate() { + self.set_label(label_x + i, label_y, ch, label_fg, false); } } } @@ -1487,6 +1597,12 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz let (Some(src), Some(dst)) = (positions.get(&edge.from), positions.get(&edge.to)) else { continue; }; + let (Some(&(src_layer, _)), Some(&(dst_layer, _))) = ( + layout.node_pos.get(&edge.from), + layout.node_pos.get(&edge.to), + ) else { + continue; + }; let route = FeedbackRouteTd { // Leave under the left border. A straight forward edge writes its // label two columns right of the box centre on the first gap row, @@ -1502,14 +1618,48 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz canvas.draw_feedback_edge_td(&route, edge_fg); if let Some(text) = edge.label.as_deref() { - // Beside this edge's lane, level with the middle of its vertical run. - let y = (route.entry_y + route.exit_y) / 2; + // Beside this edge's lane, on the middle row of whichever box the + // lane passes closest to its centre. Only node rows will do: every + // gap row in the gutter can carry the horizontal run of an outer + // lane, and a label there would cut another edge's route. + let middle = (route.entry_y + route.exit_y) / 2; + let y = (src_layer.min(dst_layer)..=src_layer.max(dst_layer)) + .map(|l| layer_top[l] + 1) + .min_by_key(|&row| row.abs_diff(middle)) + .unwrap_or(middle); labels.push((route.lane_x + 2, y, text.to_string())); } } for (x, y, text) in &labels { for (i, ch) in text.chars().enumerate() { - canvas.set(x + i, *y, ch, label_fg); + canvas.set_label(x + i, *y, ch, label_fg, false); + } + } + + // Columns where a feedback stem drops out of each layer. A straight + // forward edge writes its label along the first gap row, which is the row + // those stems drop through, so the label stops before the nearest one. + let mut stem_cols: Vec> = vec![Vec::new(); layers.len()]; + for plan in &feedback.plans { + let from = &graph.edges[plan.edge].from; + if let (Some(src), Some(&(layer, _))) = (positions.get(from), layout.node_pos.get(from)) { + stem_cols[layer].push(src.left_x() + 1); + } + } + for cols in &mut stem_cols { + cols.sort_unstable(); + cols.dedup(); + } + + // Every row a feedback route reserved, so that a forward edge whose span + // no gap budget covers can be moved off one. + let mut reserved_rows: HashSet = HashSet::new(); + for (k, &top) in layer_top.iter().enumerate() { + for r in 0..feedback.exits[k] { + reserved_rows.insert(top + 4 + r); + } + for r in 0..feedback.entries[k] { + reserved_rows.insert(top.saturating_sub(2 + r)); } } @@ -1521,7 +1671,10 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz if let (Some(src), Some(dst)) = (positions.get(&edge.from), positions.get(&edge.to)) { // Between adjacent layers the bus sits below the gap's exit rows // and above its entry rows, so neither it nor the label on the row - // above it can land on a feedback route. + // above it can land on a feedback route. An edge spanning more + // than one layer keeps the midpoint of its own span, which no gap + // budget accounts for, so it is pushed down off any reserved row + // it or its label would otherwise land on. let bus_y = match ( layout.node_pos.get(&edge.from), layout.node_pos.get(&edge.to), @@ -1529,8 +1682,24 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz (Some(&(from, _)), Some(&(to, _))) if to == from + 1 => { Some(src.bottom_y() + 3 + feedback.exits[from]) } + _ if dst.top_y > src.bottom_y() + 1 => { + let mut y = src.bottom_y() + 1 + (dst.top_y - src.bottom_y() - 1) / 2; + while y + 1 < dst.top_y + && (reserved_rows.contains(&y) || reserved_rows.contains(&(y - 1))) + { + y += 1; + } + Some(y) + } _ => None, }; + let mut label_max_x = core_width.saturating_sub(1); + if src.center_x == dst.center_x + && let Some(&(from, _)) = layout.node_pos.get(&edge.from) + && let Some(&stem) = stem_cols[from].iter().find(|&&x| x >= src.center_x + 2) + { + label_max_x = label_max_x.min(stem.saturating_sub(1)); + } canvas.draw_edge_td( src.center_x, src.bottom_y(), @@ -1540,6 +1709,7 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz edge_fg, label_fg, bus_y, + Some(label_max_x), ); } } @@ -1589,8 +1759,11 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz // The gap right of column `k` carries the drop column of every feedback // source in column k and the rise column of every feedback target in // column k+1, and still has to leave the forward edges' bend column clear - // between them. With no feedback edges the gap is the original six - // columns, so acyclic diagrams are unaffected. + // between them. The rises start two columns left of their own column + // rather than one, because the column immediately left of a box is where + // every forward arrowhead into it lands; sharing it would overwrite the + // junction. With no feedback edges the gap is the original six columns, + // so acyclic diagrams are unaffected. let gap_widths: Vec = (0..last_layer) .map(|k| { let exits = feedback.exits[k]; @@ -1598,11 +1771,11 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz node_h_gap .max(exits + entries + 4) .max(2 * exits) - .max(2 * entries + 2) + .max(2 * entries + 3) }) .collect(); // Routes into the first column or out of the last one use the margins. - let left_margin = 2.max(feedback.entries[0]); + let left_margin = 2.max(feedback.entries[0] + 1); let right_margin = 2.max(feedback.exits[last_layer]); let canvas_width: usize = left_margin @@ -1662,7 +1835,7 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz // Feedback edges go first so that forward-edge arrowheads, which overwrite // cells, end up on top of any route they cross. let lane_y = |lane: usize| core_height + 1 + lane * lane_gap; - let mut labels: Vec<(usize, usize, String)> = Vec::new(); + let mut lane_labels: Vec<(FeedbackRouteLr, String)> = Vec::new(); for plan in &feedback.plans { let edge = &graph.edges[plan.edge]; let (Some(src), Some(dst)) = (positions.get(&edge.from), positions.get(&edge.to)) else { @@ -1684,24 +1857,42 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz exit_lane_x: col_bounds[src_layer].1 + 1 + plan.src_rank, entry_x: dst.left_x() + 1, dst_bottom_y: dst.bottom_y(), - entry_lane_x: col_bounds[dst_layer].0.saturating_sub(1 + plan.dst_rank), + entry_lane_x: col_bounds[dst_layer].0.saturating_sub(2 + plan.dst_rank), lane_y: lane_y(plan.lane), }; canvas.draw_feedback_edge_lr(&route, edge_fg); if let Some(text) = edge.label.as_deref() { - // Inline on the lane, centered on its horizontal run. The run is - // all the room there is, so a longer label is cut rather than - // written over the route's own corner. - let inner = route.exit_lane_x.saturating_sub(route.entry_lane_x + 1); - let text = fit_label(text, inner); - let x = route.entry_lane_x + 1 + inner.saturating_sub(text.chars().count()) / 2; - labels.push((x, route.lane_y, text)); + lane_labels.push((route, text.to_string())); + } + } + // Lane labels go on once every route is on the canvas. An outer lane drops + // through this lane's row on its way past, and a label centred across that + // crossing would hide it, so the label takes the longest unbroken stretch + // of its own run. That stretch is all the room there is, and a longer + // label is cut rather than written over the route's own corner. + for (route, text) in &lane_labels { + let (start, room) = canvas.longest_plain_run( + route.lane_y, + route.entry_lane_x + 1, + route.exit_lane_x.saturating_sub(1), + ); + let text = fit_label(text, room); + let x = start + room.saturating_sub(text.chars().count()) / 2; + for (i, ch) in text.chars().enumerate() { + canvas.set_label(x + i, route.lane_y, ch, label_fg, true); } } - for (x, y, text) in &labels { - for (i, ch) in text.chars().enumerate() { - canvas.set(x + i, *y, ch, label_fg); + + // Every column a feedback route reserved, so that a forward edge spanning + // more than one gap can bend clear of one. + let mut reserved_cols: HashSet = HashSet::new(); + for (k, &(left, right)) in col_bounds.iter().enumerate() { + for r in 0..feedback.exits[k] { + reserved_cols.insert(right + 1 + r); + } + for r in 0..feedback.entries[k] { + reserved_cols.insert(left.saturating_sub(2 + r)); } } @@ -1713,21 +1904,39 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz if let (Some(src), Some(dst)) = (positions.get(&edge.from), positions.get(&edge.to)) { // Bend in the middle of the gap between the two columns, so that // every edge between them turns in the same column no matter how - // wide the individual nodes are. - let mid_x = match ( + // wide the individual nodes are. Between adjacent columns the gap + // budget already keeps that column clear; an edge spanning further + // is nudged right off any column a route holds. + let (mid_x, label_bounds) = match ( layout.node_pos.get(&edge.from), layout.node_pos.get(&edge.to), ) { (Some(&(src_layer, _)), Some(&(dst_layer, _))) => { let src_right = col_bounds[src_layer].1; let dst_left = col_bounds[dst_layer].0; - (dst_left > src_right + 1) + let mid = (dst_left > src_right + 1) .then(|| src_right + 1 + (dst_left - src_right - 1) / 2) + .map(|mut x| { + while x + 1 < dst_left && reserved_cols.contains(&x) { + x += 1; + } + x + }); + // Free columns for the label: right of this column's drops, + // left of the next column's rises, and clear of the + // arrowhead column just left of that column's boxes. An + // edge that spans further keeps to this first gap, because + // past it lie other columns' boxes and routes. + let next = (src_layer + 1).min(dst_layer); + let lo = src_right + 1 + feedback.exits[src_layer]; + let hi = col_bounds[next] + .0 + .saturating_sub(2 + feedback.entries[next]); + (mid, (hi >= lo).then_some((lo, hi))) } - _ => None, + _ => (None, None), }; canvas.draw_edge_lr( - src.center_x, src.right_x(), src.top_y + 1, dst.left_x(), @@ -1736,6 +1945,7 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz edge_fg, label_fg, mid_x, + label_bounds, ); } } @@ -1884,15 +2094,19 @@ mod tests { } } - let theme = Theme::dark(); - let leaked: &'static str = Box::leak(code.clone().into_boxed_str()); + let source = code.clone(); let (tx, rx) = mpsc::channel(); let handle = thread::spawn(move || { - let _ = tx.send(render_mermaid(leaked, &theme).is_some()); + let theme = Theme::dark(); + let _ = tx.send(render_mermaid(&source, &theme).is_some()); }); - if let Err(err) = rx.recv_timeout(Duration::from_secs(20)) { - let _ = handle.join(); - panic!("case {case} failed ({err:?}) for:\n{code}"); + match rx.recv_timeout(Duration::from_secs(20)) { + Ok(true) => {} + Ok(false) => panic!("case {case} did not render:\n{code}"), + Err(err) => { + let _ = handle.join(); + panic!("case {case} failed ({err:?}) for:\n{code}"); + } } handle.join().unwrap(); } @@ -1986,10 +2200,10 @@ mod tests { r#" ┌──────────┐ - ┌─YesAction 1 │───┐ + Yes┌─▶│ Action 1 │───┐ ┌───────┐ ◆────────────◆ │ └──────────┘ │ ┌─────┐ │ Start │─────▶│ Decision │───┤ ├─▶│ End │ - └───────┘ ◆────────────◆ │ No │ └─────┘ + └───────┘ ◆────────────◆ No│ │ └─────┘ │ ┌─────┐ │ └────▶│ Do │─────┘ └─────┘ @@ -2040,8 +2254,8 @@ mod tests { │ Start │─────▶│ End │ └───────┘ └─────┘ ▲ └─┐ - ┌─┘ │ - └──────────────────────┘ +┌──┘ │ +└───────────────────────┘ "#, ); @@ -2073,8 +2287,8 @@ mod tests { │ Self │ └──────┘ ▲ └─┐ - ┌─┘ │ - └────────┘ +┌──┘ │ +└─────────┘ "#, ); @@ -2124,13 +2338,13 @@ mod tests { ┌─▶│ B │───┐ ┌─────┐ │ └─────┘ │ ┌─────┐ │ A │───┤ ▲ ├─▶│ D │ - └─────┘ │ ┌─┘ │ └─────┘ - │ │┌─────┐ │ └─┐ - └─▶│ C │───┘ │ - │└─────┘ │ - │ │ - │ │ - └────────────────────┘ + └─────┘ │┌──┘ │ └─────┘ + ││ ┌─────┐ │ └─┐ + └┼▶│ C │───┘ │ + │ └─────┘ │ + │ │ + │ │ + └─────────────────────┘ "#, ); @@ -2149,13 +2363,13 @@ mod tests { ┌──────▶│ B │───────┐ ┌─────┐ │ └─────┘ │ ┌─────┐ │ A │───┤ ▲ ├─▶│ D │ - └─────┘ │ ┌──────┘ │ └─────┘ - │ │┌──────────────┐ │ └─┐ - └─▶│ VeryLongName │───┘ │ - │└──────────────┘ │ - │ │ - │ │ - └─────────────────────────────┘ + └─────┘ │┌───────┘ │ └─────┘ + ││ ┌──────────────┐ │ └─┐ + └┼▶│ VeryLongName │───┘ │ + │ └──────────────┘ │ + │ │ + │ │ + └──────────────────────────────┘ "#, ); @@ -2178,11 +2392,11 @@ mod tests { │ A │───┤ └─────┐ ├─▶│ D │ └─────┘ │ │ │ └─────┘ ▲ │ ┌──────────────┐│ │ - ┌─┘ └─▶│ VeryLongName │┼──┘ - │ └──────────────┘│ - │ │ - │ │ - └─────────────────────────────┘ +┌──┘ └─▶│ VeryLongName │┼──┘ +│ └──────────────┘│ +│ │ +│ │ +└──────────────────────────────┘ "#, ); @@ -2251,13 +2465,13 @@ mod tests { │ S │───┤ └──┐ └─────┘ │ │ ▲ │ ┌─────┐ │ - ┌─┘ └─▶│ B │ │ - │ └─────┘ │ - │ └─┐│ - │ ││ - ├────────────────────┼┘ - │ │ - └────────────────────┘ +┌──┘ └─▶│ B │ │ +│ └─────┘ │ +│ └─┐│ +│ ││ +├─────────────────────┼┘ +│ │ +└─────────────────────┘ "#, ); @@ -2323,10 +2537,10 @@ mod tests { │ ┌─────┐ │ ▼ ▼ │ │ ┌───────┐ │ │ - │ Check │ │ │ + │ Check │ │ again │ retry └───────┘ │ │ │ │ │ - │ │ again │ retry + │ │ │ │ │ │ ▼ │ │ ┌──────┐ │ │ @@ -2356,10 +2570,10 @@ mod tests { │ Start │─────▶│ Check │─────▶│ Work │─────▶│ Done │ └───────┘ └───────┘ └──────┘ └──────┘ ▲ ▲ └─┐ └─┐ - ┌─┘ ┌─┘ │ │ - │ └─────────again─────────┘ │ - │ │ - └───────────────────────retry────────────────────────┘ +┌──┘ ┌──┘ │ │ +│ └─────────again──────────┘ │ +│ │ +└────────────────────────retry────────────────────────┘ "#, ); @@ -2378,8 +2592,8 @@ mod tests { │ A │ └─────┘ ▲ └─┐ - ┌─┘ │ - └this i…┘ +┌──┘ │ +└this is…┘ "#, ); @@ -2450,6 +2664,175 @@ mod tests { ┌─────┐ │ E │ └─────┘ +"#, + ); + } + + #[test] + fn feedback_label_clears_an_outer_lane_top_down() { + // `back` belongs to the inner lane. Every gap row beside it carries the + // horizontal run of an outer lane, so the label sits on a node row. + assert_render( + "graph TD\n A --> B\n B --> X\n B --> Y\n X --> Z\n Y --> Z\n Z -->|back| X\n X --> A\n Y --> A\n", + r#" + ┌──────────────────┬───┐ + ▼ │ │ + ┌─────┐ │ │ + │ A │ │ │ + └─────┘ │ │ + │ │ │ + │ │ │ + │ │ │ + ▼ │ │ + ┌─────┐ │ │ + │ B │ │ │ + └─────┘ │ │ + │ │ │ + │ │ │ + ┌─────┴────┐ │ │ + │ ┌────────┼───────┐ │ │ + ▼ ▼ ▼ │ │ │ + ┌─────┐ ┌─────┐ │ │ │ + │ X │ │ Y │ │ back │ │ + └─────┘ └─────┘ │ │ │ + │ │ │ │ │ │ │ + │ │ └─┼───────┼───────┼───┘ + └─┼──────────┼───────┼───────┘ + │ │ │ + └─────┬────┘ │ + ▼ │ + ┌─────┐ │ + │ Z │ │ + └─────┘ │ + │ │ + └──────────────┘ +"#, + ); + } + + #[test] + fn a_long_label_cannot_reach_the_gutter_lanes_top_down() { + // The label runs along the first gap row towards the gutter, where the + // lane of the back edge is. It is cut before it gets there. + assert_render( + "graph TD\n A -->|a very long edge label| B\n B --> A\n", + r#" + ┌─────┐ + ▼ │ + ┌─────┐ │ + │ A │ │ + └─────┘ │ + │ a ve… │ + │ │ + │ │ + ▼ │ + ┌─────┐ │ + │ B │ │ + └─────┘ │ + │ │ + └─────────┘ +"#, + ); + } + + #[test] + fn a_long_label_stops_before_a_sibling_feedback_stem_top_down() { + // B's label runs right along the first gap row, which is the row C's + // feedback stem drops through. + assert_render( + "graph TD\n A --> B\n A --> C\n B -->|a long label| D\n C --> E\n C --> A\n", + r#" + ┌──────────┐ + ▼ │ + ┌─────┐ │ + │ A │ │ + └─────┘ │ + │ │ + │ │ + ┌─────┴────┐ │ + ▼ ▼ │ + ┌─────┐ ┌─────┐ │ + │ B │ │ C │ │ + └─────┘ └─────┘ │ + │ a long…│ │ │ + │ └─┼───────┘ + │ │ + │ │ + ▼ ▼ + ┌─────┐ ┌─────┐ + │ D │ │ E │ + └─────┘ └─────┘ +"#, + ); + } + + #[test] + fn bent_forward_label_clears_the_feedback_rise_left_right() { + // `no` would sit right of the bend, which is where the route into B + // rises. There is room left of the bend, so it goes there instead. + assert_render( + "graph LR\n A --> B\n A -->|no| C\n B --> D\n C --> D\n D --> B\n", + r#" + + ┌─────┐ + ┌─▶│ B │───┐ + ┌─────┐ │ └─────┘ │ ┌─────┐ + │ A │───┤ ▲ ├─▶│ D │ + └─────┘ no│┌──┘ │ └─────┘ + ││ ┌─────┐ │ └─┐ + └┼▶│ C │───┘ │ + │ └─────┘ │ + │ │ + │ │ + └─────────────────────┘ + +"#, + ); + } + + #[test] + fn straight_forward_label_clears_the_feedback_drop_left_right() { + // Y's label starts two columns right of its box, which is the column X + // drops through on its way to the lane. It starts past the drops. + assert_render( + "graph LR\n S --> X\n S --> Y\n X --> S\n Y --> S\n X --> Z1\n Y -->|lbl| Z2\n", + r#" + + ┌─────┐ ┌─────┐ + ┌─▶│ X │─────▶│ Z1 │ + ┌─────┐ │ └─────┘ └─────┘ + │ S │───┤ └──┐ + └─────┘ │ │ + ▲ │ ┌─────┐ │lbl ┌─────┐ +┌──┘ └─▶│ Y │─┼───▶│ Z2 │ +│ └─────┘ │ └─────┘ +│ └─┐│ +│ ││ +├─────────────────────┼┘ +│ │ +└─────────────────────┘ + +"#, + ); + } + + #[test] + fn lane_label_keeps_clear_of_a_crossing_left_right() { + // The outer lane drops through this lane's run. The label takes the + // longest unbroken stretch instead of covering the crossing. + assert_render( + "graph LR\n A --> B\n B --> C\n C --> D\n D -->|label here| B\n C --> A\n", + r#" + + ┌─────┐ ┌─────┐ ┌─────┐ ┌─────┐ + │ A │─────▶│ B │─────▶│ C │─────▶│ D │ + └─────┘ └─────┘ └─────┘ └─────┘ + ▲ ▲ └─┐ └─┐ +┌──┘ ┌──┘ │ │ +│ └─────label here──────┼────────────┘ +│ │ +└──────────────────────────────────┘ + "#, ); } @@ -2476,10 +2859,10 @@ mod tests { │ ┌─────┐ │ ▼ ▼ │ │ ┌───────┐ │ │ - │ Check │ │ │ + │ Check │ │ a very long label │ x └───────┘ │ │ │ │ │ - │ │ a very long label │ x + │ │ │ │ │ │ ▼ │ │ ┌──────┐ │ │ diff --git a/src/json.rs b/src/json.rs index f2f3119..64de7ee 100644 --- a/src/json.rs +++ b/src/json.rs @@ -2117,7 +2117,6 @@ pub fn render_diagram( }; canvas.draw_edge_lr( - src_pos.left_x + src_pos.width / 2, src_right_x, src_cy, dst_left_x, @@ -2126,6 +2125,7 @@ pub fn render_diagram( edge_fg, None, mid_x_override, + None, ); } } From 73b5ccea63d932378ba12443387caa8f474d7467 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Mon, 7 Sep 2026 16:58:55 +0530 Subject: [PATCH 05/17] refactor(diagram): check render invariants on the canvas, not in draw calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The feedback-routing invariants were asserted from inside the drawing functions, which meant `set_label` grew an `on_lane: bool` parameter that production code never reads. There is even a `#[cfg(not(test))] let _ = on_lane;` sitting in there to stop the compiler complaining about the thing. That is backwards. The caller was being asked to declare what it was allowed to paint over, so the check ended up trusting precisely the code it existed to check. Put the permission on the cell instead. A gutter lane marks its own horizontal run as it draws it, and that run is the one kind of edge cell a label may legitimately cover, so `set_label` can look at what is actually underneath it rather than take the caller's word for it. The route-through-a-box check moves out to `assert_invariants`, run once when a render finishes. That one really does belong after the fact: `add_connection` silently skips node cells, so a route drawn over a box leaves nothing to catch at the moment of drawing — just a line that stops dead at a border and reads as an edge nobody wrote. No behavior change. Every snapshot renders byte-for-byte identical. Claude-Session: https://claude.ai/code/session_01YSfTy6Fz4tGed1Fpe8ESe7 --- src/diagram.rs | 101 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 66 insertions(+), 35 deletions(-) diff --git a/src/diagram.rs b/src/diagram.rs index 3140349..ddd3a2a 100644 --- a/src/diagram.rs +++ b/src/diagram.rs @@ -754,9 +754,12 @@ pub(crate) struct CanvasCell { pub(crate) bg: Option, pub(crate) is_node: bool, pub(crate) connects: u8, - /// Drawn by a feedback route. Labels go on last and overwrite whatever is - /// under them, so this is what [`Canvas::set_label`] checks against. + /// Drawn by a feedback route. is_feedback: bool, + /// Part of a left-right gutter lane's plain horizontal run. A lane carries + /// its own label inline, so this is the one kind of edge cell a label may + /// be written over. + is_lane: bool, } impl Default for CanvasCell { @@ -768,6 +771,7 @@ impl Default for CanvasCell { is_node: false, connects: 0, is_feedback: false, + is_lane: false, } } } @@ -818,47 +822,65 @@ impl Canvas { /// Draw one cell of a feedback route. /// /// Feedback routes are planned to run only through gap rows and gap - /// columns, which never contain nodes. `add_connection` would silently - /// skip a node cell and leave a route that looks like it stops at a box, - /// so under test a violation of that invariant fails instead. + /// columns, which never contain nodes. `add_connection` silently skips a + /// node cell, so a route drawn over a box leaves a line that stops dead at + /// the border rather than an error; the cell is marked either way and + /// [`Canvas::assert_invariants`] catches it after the render. fn connect_route(&mut self, x: usize, y: usize, dir: u8, fg: Option) { - #[cfg(test)] - if y < self.height && x < self.width { - assert!( - !self.cells[y][x].is_node, - "feedback route runs through the node cell at ({x}, {y})" - ); - } self.add_connection(x, y, dir, fg); if y < self.height && x < self.width { self.cells[y][x].is_feedback = true; } } + /// Mark a cell as part of a left-right gutter lane's plain horizontal run, + /// the one kind of edge cell that lane's own label may be written over. + fn mark_lane(&mut self, x: usize, y: usize) { + if y < self.height && x < self.width { + self.cells[y][x].is_lane = true; + } + } + /// Write one character of a label. /// - /// Labels are drawn after the routes and overwrite whatever is under them, - /// so one placed badly hides a route instead of the other way round. Under - /// test the only cells a label may take are ones no feedback route drew, - /// plus, for a label riding inline on a left-right lane, that lane's own - /// plain horizontal run. - fn set_label(&mut self, x: usize, y: usize, ch: char, fg: Option, on_lane: bool) { - // Only the assertion below reads `on_lane`, and it is test-only. - #[cfg(not(test))] - let _ = on_lane; + /// Labels overwrite whatever is under them, so one placed badly hides a + /// route instead of the other way round. Under test a label may not take a + /// feedback route's cell, the one exception being a lane's own label + /// riding inline on that lane's plain horizontal run. The permission comes + /// from the cell rather than from the caller, so what a label may cover is + /// decided by what is actually on the canvas under it. + fn set_label(&mut self, x: usize, y: usize, ch: char, fg: Option) { #[cfg(test)] if y < self.height && x < self.width { let cell = &self.cells[y][x]; - let allowed = !cell.is_feedback - || (on_lane && !cell.is_node && cell.connects == (CONN_LEFT | CONN_RIGHT)); assert!( - allowed, - "label character {ch:?} lands on the feedback route cell at ({x}, {y})" + !cell.is_feedback || (cell.is_lane && cell.connects == (CONN_LEFT | CONN_RIGHT)), + "label {ch:?} hides the feedback route at ({x}, {y})" ); } self.set(x, y, ch, fg); } + /// Check, once a render is finished, that no feedback route was laid over + /// a box. + /// + /// `add_connection` silently skips a node cell, so a route planned through + /// a box does not fail: it renders as a line that stops dead at the border, + /// which reads as an edge the source never declared. `connect_route` marks + /// the cell whether or not the character landed, so the collision is still + /// here to be found afterwards. + #[cfg(test)] + fn assert_invariants(&self) { + for (y, row) in self.cells.iter().enumerate() { + for (x, cell) in row.iter().enumerate() { + assert!( + !(cell.is_feedback && cell.is_node), + "feedback route runs through the node cell at ({x}, {y})" + ); + } + } + } + /// True when the cell carries a plain horizontal run and nothing else, so /// a label may be written over it without hiding a corner, a crossing or /// a box. @@ -1118,7 +1140,7 @@ impl Canvas { let label_x = src_cx + 2; let room = label_max_x.map_or(usize::MAX, |hi| (hi + 1).saturating_sub(label_x)); for (i, ch) in fit_label(text, room).chars().enumerate() { - self.set_label(label_x + i, label_y, ch, label_fg, false); + self.set_label(label_x + i, label_y, ch, label_fg); } } } else { @@ -1171,7 +1193,7 @@ impl Canvas { for (i, ch) in fit_label(text, room).chars().enumerate() { let lx = label_start + i; if lx < self.width { - self.set_label(lx, label_y, ch, label_fg, false); + self.set_label(lx, label_y, ch, label_fg); } } } @@ -1223,7 +1245,7 @@ impl Canvas { None => (src_right_x + 2, usize::MAX), }; for (i, ch) in fit_label(text, room).chars().enumerate() { - self.set_label(label_x + i, label_y, ch, label_fg, false); + self.set_label(label_x + i, label_y, ch, label_fg); } } } else { @@ -1292,7 +1314,7 @@ impl Canvas { None => (mid_x + 2, usize::MAX), }; for (i, ch) in fit_label(text, room).chars().enumerate() { - self.set_label(label_x + i, label_y, ch, label_fg, false); + self.set_label(label_x + i, label_y, ch, label_fg); } } } @@ -1393,6 +1415,7 @@ impl Canvas { self.connect_route(r.exit_lane_x, r.lane_y, CONN_UP | CONN_LEFT, fg); for x in (r.entry_lane_x + 1)..r.exit_lane_x { self.connect_route(x, r.lane_y, CONN_LEFT | CONN_RIGHT, fg); + self.mark_lane(x, r.lane_y); } self.connect_route(r.entry_lane_x, r.lane_y, CONN_UP | CONN_RIGHT, fg); @@ -1632,7 +1655,7 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz } for (x, y, text) in &labels { for (i, ch) in text.chars().enumerate() { - canvas.set_label(x + i, *y, ch, label_fg, false); + canvas.set_label(x + i, *y, ch, label_fg); } } @@ -1714,6 +1737,9 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz } } + #[cfg(test)] + canvas.assert_invariants(); + let rows = canvas.to_span_rows(theme); Some((rows, canvas_width)) } @@ -1880,7 +1906,7 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz let text = fit_label(text, room); let x = start + room.saturating_sub(text.chars().count()) / 2; for (i, ch) in text.chars().enumerate() { - canvas.set_label(x + i, route.lane_y, ch, label_fg, true); + canvas.set_label(x + i, route.lane_y, ch, label_fg); } } @@ -1950,6 +1976,9 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz } } + #[cfg(test)] + canvas.assert_invariants(); + let rows = canvas.to_span_rows(theme); Some((rows, canvas_width)) } @@ -2009,9 +2038,10 @@ mod tests { /// Compare a render against a snapshot written at column 0 inside a raw /// string literal (one leading newline, trailing blank rows ignored). /// - /// Every render also checks an invariant of its own: [`Canvas::connect_route`] - /// fails the test if a feedback route is laid over a node cell, which is - /// what a route that runs through a box looks like. + /// Every render also checks an invariant of its own: + /// [`Canvas::assert_invariants`] fails the test if a feedback route was + /// laid over a node cell, which is what a route that runs through a box + /// looks like. fn assert_render(code: &'static str, expected: &str) { let text = render_text_with_timeout(code); let expected = expected.strip_prefix('\n').unwrap_or(expected); @@ -2061,7 +2091,8 @@ mod tests { } /// Render a deterministic spread of graph shapes and let - /// [`Canvas::connect_route`] check every feedback route against every box. + /// [`Canvas::assert_invariants`] check every feedback route against every + /// box. /// /// The shapes vary in node count, node width, edge count, direction and /// labelling, because a route only collides with a box that a *differently From 5e46f4fd4f9a0c9b76da168db37108a0d31d3316 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Mon, 7 Sep 2026 16:59:50 +0530 Subject: [PATCH 06/17] fix(diagram): size left-right gaps for the labels they carry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewing the feedback-edge work turned up something worse than the bug it was chasing. An ordinary labelled flowchart — graph LR A[Start] --> B{Check} B -->|success| C[Deploy] — rendered that edge as `su…`. Two characters and an ellipsis. The gap between two columns was budgeted for the feedback drops and rises it has to carry, and for nothing else. It stays six columns wide, the bend sits in the middle of it, and a bent label has to fit on one side of that bend. Three columns is all a label ever got. Anything longer than "Yes" was cut. For the record `main` doesn't get this right either. It just fails in the other direction and paints the label straight through the destination box. Neither of those is a label. So budget the gap for its labels too. A bent one needs twice its own width because it lives beside a bend in the middle of the gap; a straight one runs the length of it. Diagrams with long edge labels come out wider, and that is the right trade — a wide diagram says what it means, and a narrow one saying `su…` does not. Two smaller things fell out of the same review. A bent label picked its row by splitting the difference between the two endpoints, which lands on the source's own row whenever the two are two rows apart — and that is exactly where the edge's outgoing run is. `│ A │lbl┐`, the label eating the line it was naming. Any row strictly between the two runs is clear of both, so use one of those; when the rows are adjacent there is no such row, and the label takes whichever side of the bend the run on its own row cannot reach. And `fit_label` would hand back a bare `…` given a single column, which tells the reader nothing except that something used to be there. Under three columns the label is dropped instead. While at it, write down why a top-down feedback stem leaves under the left border and crosses its own source's outgoing edge. That is a deliberate trade — a crossing reads as two edges meeting, which is what it is — and without a note someone will helpfully "fix" it back. Not fixed, and not a regression: in a dense diagram a label can still land on a *different* edge's line. That predates all of this routing work and wants a real placement pass, not another special case. Claude-Session: https://claude.ai/code/session_01YSfTy6Fz4tGed1Fpe8ESe7 --- src/diagram.rs | 206 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 179 insertions(+), 27 deletions(-) diff --git a/src/diagram.rs b/src/diagram.rs index ddd3a2a..a0f8972 100644 --- a/src/diagram.rs +++ b/src/diagram.rs @@ -704,19 +704,23 @@ pub(crate) fn label_box_width(label: &str, shape: NodeShape) -> usize { /// Some routes have a fixed amount of room for their label (a left-right /// lane runs between two node columns), and a label that overran it used to /// paint over the route's own corner and off the edge of the canvas. +/// +/// Below three columns there is no room for a cut label that still says +/// anything: one character and an ellipsis reads as a different word, and a +/// bare ellipsis says only that something was dropped. The label is left out +/// altogether instead, which at least does not misname the edge. pub(crate) fn fit_label(label: &str, width: usize) -> String { if label.chars().count() <= width { return label.to_string(); } - match width { - 0 => String::new(), - 1 => "\u{2026}".to_string(), - _ => label - .chars() - .take(width - 1) - .chain(std::iter::once('\u{2026}')) - .collect(), + if width < 3 { + return String::new(); } + label + .chars() + .take(width - 1) + .chain(std::iter::once('\u{2026}')) + .collect() } // ───── Canvas ───── @@ -849,6 +853,11 @@ impl Canvas { /// riding inline on that lane's plain horizontal run. The permission comes /// from the cell rather than from the caller, so what a label may cover is /// decided by what is actually on the canvas under it. + /// + /// Forward edges are deliberately outside this check. A label can still + /// land on a *different* forward edge's line in a dense diagram, which + /// predates the feedback routing and is not addressed here; a label + /// covering its own edge's line is checked in [`Canvas::draw_edge_lr`]. fn set_label(&mut self, x: usize, y: usize, ch: char, fg: Option) { #[cfg(test)] if y < self.height && x < self.width { @@ -1292,15 +1301,35 @@ impl Canvas { // there is room for it, otherwise left of the bend, and cut when // neither side can hold it. Running past the free columns would // hide a feedback route or the destination box. + // + // The edge's own two horizontal runs are what the label row has to + // clear. One sits on `src_cy`, from the source out to the bend; the + // other on `dst_cy`, from the bend in to the destination. Any row + // strictly between them is clear of both, so that is where the + // label goes. Rows one apart leave no such row, and there the label + // takes whichever side of the bend the run on its own row does not + // reach. if let Some(text) = label { - let label_y = min_y + (max_y - min_y).saturating_sub(1) / 2; + let (label_y, allow_left, allow_right) = if max_y - min_y >= 2 { + ((min_y + (max_y - min_y - 1) / 2).max(min_y + 1), true, true) + } else { + (min_y, min_y != src_cy, min_y != dst_cy) + }; let len = text.chars().count(); let (label_x, room) = match label_bounds { Some((lo, hi)) => { let right_x = (mid_x + 2).max(lo); - let right_room = (hi + 1).saturating_sub(right_x); + let right_room = if allow_right { + (hi + 1).saturating_sub(right_x) + } else { + 0 + }; let left_hi = mid_x.saturating_sub(1).min(hi); - let left_room = (left_hi + 1).saturating_sub(lo); + let left_room = if allow_left { + (left_hi + 1).saturating_sub(lo) + } else { + 0 + }; if len <= right_room { (right_x, right_room) } else if len <= left_room { @@ -1311,9 +1340,27 @@ impl Canvas { (lo, left_room) } } - None => (mid_x + 2, usize::MAX), + None if allow_right => (mid_x + 2, usize::MAX), + None => (mid_x.saturating_sub(1 + len), len), }; - for (i, ch) in fit_label(text, room).chars().enumerate() { + let fitted = fit_label(text, room); + // Whichever side it took, the label has to clear this edge's + // own two horizontal runs: the one on `src_cy` reaching out to + // the bend, and the one on `dst_cy` coming back in from it. A + // label written over either erases the edge it names. + #[cfg(test)] + if !fitted.is_empty() { + let clear = if label_x > mid_x { + label_y != dst_cy + } else { + label_y != src_cy + }; + assert!( + clear, + "label {fitted:?} sits on the edge's own run at ({label_x}, {label_y})" + ); + } + for (i, ch) in fitted.chars().enumerate() { self.set_label(label_x + i, label_y, ch, label_fg); } } @@ -1630,6 +1677,12 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz // Leave under the left border. A straight forward edge writes its // label two columns right of the box centre on the first gap row, // which is the row this stem drops through. + // + // The cost is a crossing: a source that also has forward children + // drops this stem across its own outgoing edge, drawn as `┼`. That + // is deliberate. A crossing reads as two edges that meet, which is + // what it is, whereas a label written over the stem would hide one + // of them entirely. exit_x: src.left_x() + 1, src_bottom_y: src.bottom_y(), exit_y: src.bottom_y() + 2 + plan.src_rank, @@ -1780,6 +1833,26 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz let max_nodes_in_layer = layers.iter().map(|l| l.len()).max().unwrap_or(1); + // Longest forward-edge label that each gap has to hold. A label lives in + // the gap right of its source's column, whether the edge ends in the next + // column or a later one, so it is charged to that gap. + let mut label_room: Vec = vec![0; last_layer]; + for (idx, edge) in graph.edges.iter().enumerate() { + if layout.feedback_set.contains(&idx) { + continue; + } + let (Some(text), Some(&(src_layer, _)), Some(&(dst_layer, _))) = ( + edge.label.as_deref(), + layout.node_pos.get(&edge.from), + layout.node_pos.get(&edge.to), + ) else { + continue; + }; + if src_layer < last_layer && dst_layer > src_layer { + label_room[src_layer] = label_room[src_layer].max(text.chars().count()); + } + } + // ── Column budget ── // // The gap right of column `k` carries the drop column of every feedback @@ -1790,14 +1863,31 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz // every forward arrowhead into it lands; sharing it would overwrite the // junction. With no feedback edges the gap is the original six columns, // so acyclic diagrams are unaffected. + // + // A gap that carries a labelled forward edge also has to be wide enough + // for the label. A bent edge writes its label on one side of its bend, + // which sits at the middle of the gap, so it needs twice its own width + // past the drop columns; a straight edge writes along the gap, between the + // drops and the rises. Sizing for that is what keeps a label readable: + // with a fixed six-column gap anything longer than three characters was + // cut to an ellipsis, and the edge stopped saying what it meant. let gap_widths: Vec = (0..last_layer) .map(|k| { let exits = feedback.exits[k]; let entries = feedback.entries[k + 1]; + // A bent label is right-aligned against the bend, so it needs one + // column of padding as well; without it the text butts against the + // node border and reads as part of the box. + let bent = match label_room[k] { + 0 => 0, + label => 2 * (label + exits + 1), + }; node_h_gap .max(exits + entries + 4) .max(2 * exits) .max(2 * entries + 3) + .max(bent) + .max(label_room[k] + entries + 2) }) .collect(); // Routes into the first column or out of the last one use the margins. @@ -2230,14 +2320,14 @@ mod tests { "graph LR\n A[Start] --> B{Decision}\n B -->|Yes| C[Action 1]\n B -->|No| D[Do]\n C --> E[End]\n D --> E\n", r#" - ┌──────────┐ - Yes┌─▶│ Action 1 │───┐ - ┌───────┐ ◆────────────◆ │ └──────────┘ │ ┌─────┐ - │ Start │─────▶│ Decision │───┤ ├─▶│ End │ - └───────┘ ◆────────────◆ No│ │ └─────┘ - │ ┌─────┐ │ - └────▶│ Do │─────┘ - └─────┘ + ┌──────────┐ + ┌──▶│ Action 1 │───┐ + ┌───────┐ ◆────────────◆ Yes│ └──────────┘ │ ┌─────┐ + │ Start │─────▶│ Decision │────┤ ├─▶│ End │ + └───────┘ ◆────────────◆ No│ │ └─────┘ + │ ┌─────┐ │ + └─────▶│ Do │─────┘ + └─────┘ "#, ); @@ -2829,14 +2919,14 @@ mod tests { "graph LR\n S --> X\n S --> Y\n X --> S\n Y --> S\n X --> Z1\n Y -->|lbl| Z2\n", r#" - ┌─────┐ ┌─────┐ - ┌─▶│ X │─────▶│ Z1 │ - ┌─────┐ │ └─────┘ └─────┘ + ┌─────┐ ┌─────┐ + ┌─▶│ X │───────────▶│ Z1 │ + ┌─────┐ │ └─────┘ └─────┘ │ S │───┤ └──┐ └─────┘ │ │ - ▲ │ ┌─────┐ │lbl ┌─────┐ -┌──┘ └─▶│ Y │─┼───▶│ Z2 │ -│ └─────┘ │ └─────┘ + ▲ │ ┌─────┐ │lbl ┌─────┐ +┌──┘ └─▶│ Y │─┼─────────▶│ Z2 │ +│ └─────┘ │ └─────┘ │ └─┐│ │ ││ ├─────────────────────┼┘ @@ -2868,6 +2958,68 @@ mod tests { ); } + #[test] + fn a_forward_label_is_not_cut_to_an_ellipsis_left_right() { + // The gap between two columns is sized for the label it has to carry. + // At a fixed six columns a bent label had three columns to sit in, so + // anything longer came out as two characters and an ellipsis and the + // edge stopped saying what it meant. + assert_render( + "graph LR\n A[Start] --> B{Check}\n B -->|success| C[Deploy]\n B -->|failure| D[Rollback]\n", + r#" + + ┌────────┐ + ┌───────▶│ Deploy │ + ┌───────┐ ◆─────────◆ success│ └────────┘ + │ Start │─────▶│ Check │────────┤ + └───────┘ ◆─────────◆ failure│ + │ ┌──────────┐ + └──────▶│ Rollback │ + └──────────┘ + +"#, + ); + } + + #[test] + fn bent_forward_label_clears_the_edges_own_run_left_right() { + // A -> X rises by two rows, so the label row used to land on A's own + // horizontal run and paint over it: `│ A │lbl┐`. It goes on the row + // between the two runs instead. + assert_render( + "graph LR\n A -->|lbl| X\n B --> X\n B --> Y\n C --> Y\n", + r#" + + ┌─────┐ + │ A │────┐ + └─────┘ lbl│ ┌─────┐ + ├──▶│ X │ + │ └─────┘ + ┌─────┐ │ + │ B │────┤ + └─────┘ │ ┌─────┐ + ├──▶│ Y │ + │ └─────┘ + ┌─────┐ │ + │ C │────┘ + └─────┘ + +"#, + ); + } + + #[test] + fn a_label_with_no_room_to_be_cut_is_dropped() { + assert_eq!(fit_label("retry", 5), "retry"); + assert_eq!(fit_label("retry", 4), "ret\u{2026}"); + assert_eq!(fit_label("retry", 3), "re\u{2026}"); + // Below three columns a cut label names the edge something it is not, + // and a bare ellipsis says only that something was dropped. + assert_eq!(fit_label("retry", 2), ""); + assert_eq!(fit_label("retry", 1), ""); + assert_eq!(fit_label("retry", 0), ""); + } + #[test] fn one_long_label_does_not_widen_every_lane() { let code = "graph TD\n Start --> Check\n Check --> Work\n Work --> Done\n Done -->|x| Start\n Work -->|a very long label| Check\n"; From d279b6ad8957f8ec25f88102a6c7f76b7101dd4e Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Tue, 8 Sep 2026 19:01:46 +0530 Subject: [PATCH 07/17] refactor(diagram): count feedback endpoints from the edges that get drawn The gap budget reserves a row or column for every feedback endpoint in a layer, and the routes are placed by their rank among those endpoints. Both numbers were counted straight off the feedback set, before the planner had decided which of those edges it could actually place. An edge whose endpoints are not both in a layer is silently dropped a few lines later. Today nothing can trigger that, because every edge endpoint is a registered node, so the two counts happen to agree. But they agree by luck rather than by construction, and if the drop ever starts firing the budget will be sized for a route nobody draws while the ranks are numbered as if it existed. Good luck debugging that from a picture of a box with a line through it. Resolve the endpoints once, up front, into an explicit list of the edges that survived, and count from that. Same output, one fewer way for the geometry to disagree with itself later. --- src/diagram.rs | 65 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 22 deletions(-) diff --git a/src/diagram.rs b/src/diagram.rs index a0f8972..e6a3b53 100644 --- a/src/diagram.rs +++ b/src/diagram.rs @@ -366,41 +366,62 @@ struct FeedbackPlans { entries: Vec, } +/// A feedback edge whose two endpoints both landed in a layer, with where they +/// landed. Planning resolves the endpoints once into this, so the per-layer +/// endpoint counts that size the gaps and the ranks that place the routes are +/// taken from the same set of edges the renderer goes on to draw. +struct PlacedFeedback { + /// Index into `graph.edges`. + edge: usize, + src_layer: usize, + src_pos: usize, + dst_layer: usize, + dst_pos: usize, +} + fn plan_feedback(graph: &Graph, layout: &Layout) -> FeedbackPlans { let layer_count = layout.layers.len(); - let mut sources: Vec> = vec![BTreeSet::new(); layer_count]; - let mut targets: Vec> = vec![BTreeSet::new(); layer_count]; - for &idx in &layout.feedback { - let edge = &graph.edges[idx]; - if let Some(&(layer, pos)) = layout.node_pos.get(&edge.from) { - sources[layer].insert(pos); - } - if let Some(&(layer, pos)) = layout.node_pos.get(&edge.to) { - targets[layer].insert(pos); - } - } - // Rank counts the endpoints between this one and the gutter. - let rank = |positions: &BTreeSet, pos: usize| positions.range(pos + 1..).count(); - - let mut plans: Vec = layout + // Resolve every feedback edge to its endpoints first. An edge whose + // endpoints are not both placed is dropped here, so the per-layer counts + // below are taken from the routes that are actually drawn: the gap budget + // and the ranks can never be sized for a route the renderer skips. + let placed: Vec = layout .feedback .iter() .filter_map(|&idx| { let edge = &graph.edges[idx]; let &(src_layer, src_pos) = layout.node_pos.get(&edge.from)?; let &(dst_layer, dst_pos) = layout.node_pos.get(&edge.to)?; - Some(FeedbackPlan { + Some(PlacedFeedback { edge: idx, - lane: 0, - src_rank: rank(&sources[src_layer], src_pos), - dst_rank: rank(&targets[dst_layer], dst_pos), + src_layer, + src_pos, + dst_layer, + dst_pos, }) }) .collect(); - // Lanes are numbered over the edges that survived, so they stay contiguous. - for (lane, plan) in plans.iter_mut().enumerate() { - plan.lane = lane; + + let mut sources: Vec> = vec![BTreeSet::new(); layer_count]; + let mut targets: Vec> = vec![BTreeSet::new(); layer_count]; + for p in &placed { + sources[p.src_layer].insert(p.src_pos); + targets[p.dst_layer].insert(p.dst_pos); } + // Rank counts the endpoints between this one and the gutter. + let rank = |positions: &BTreeSet, pos: usize| positions.range(pos + 1..).count(); + + // Lanes are numbered over the edges that survived, so they stay contiguous. + let plans: Vec = placed + .iter() + .enumerate() + .map(|(lane, p)| FeedbackPlan { + edge: p.edge, + lane, + src_rank: rank(&sources[p.src_layer], p.src_pos), + dst_rank: rank(&targets[p.dst_layer], p.dst_pos), + }) + .collect(); FeedbackPlans { plans, From 77fc326922dc623483c1d7299c693ba79ef5efce Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Tue, 8 Sep 2026 19:02:14 +0530 Subject: [PATCH 08/17] fix(diagram): move the left-right bend with the label, not the whole gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit in this branch sized each left-right gap for the labels it carries, because a fixed six-column gap cut anything longer than `Yes` down to an ellipsis. That was the right problem to solve. The way it got solved was not. The bend sat at the middle of the gap, and a bent edge writes its label on one side of that bend. So buying N columns of room for the label meant buying 2N columns of gap. An eighteen-character label cost thirty-eight columns, and `wrap_lines` has no exemption for diagram rows: anything wider than the terminal gets word-wrapped into fragments. We traded a truncated label for a shredded diagram. Nothing actually requires the bend to sit at the middle. All it has to do is be the *same* column for every edge crossing that gap, so the runs converge into one junction instead of fanning out. So put the label between the drop columns and the bend, and let the bend sit wherever that leaves it. A gap now holds the drops, a column of padding, the label, the bend, a column of run, the rises, and the arrowhead — which is one term, linear in the label, instead of two terms and a doubling. While at it, that one term charges for the drop columns, which the old straight-label term forgot. It was covered anyway, because the bent term's doubling was big enough to hide the shortfall in every shape I could construct. Being accidentally correct because a *different* wrong number happened to be larger is not a property I want to rely on, so the term now stands on its own. Diagrams with long labels come out roughly a third narrower. Top-down renders are untouched. --- src/diagram.rs | 212 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 149 insertions(+), 63 deletions(-) diff --git a/src/diagram.rs b/src/diagram.rs index e6a3b53..3faf331 100644 --- a/src/diagram.rs +++ b/src/diagram.rs @@ -1886,29 +1886,32 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz // so acyclic diagrams are unaffected. // // A gap that carries a labelled forward edge also has to be wide enough - // for the label. A bent edge writes its label on one side of its bend, - // which sits at the middle of the gap, so it needs twice its own width - // past the drop columns; a straight edge writes along the gap, between the - // drops and the rises. Sizing for that is what keeps a label readable: - // with a fixed six-column gap anything longer than three characters was - // cut to an ellipsis, and the edge stopped saying what it meant. + // for the label. Both a straight label and a bent one are written between + // the drop columns and the bend, so such a gap holds, left to right: the + // drop columns, one column of padding so the text does not butt against + // the node border, the label, the bend, one column of run, the rise + // columns, and the arrowhead column. Sizing for that is what keeps a label + // readable: with a fixed six-column gap anything longer than three + // characters was cut to an ellipsis, and the edge stopped saying what it + // meant. + // + // The bend moves right with the label rather than staying at the middle of + // the gap (see `bend_x` below). Holding it at the middle would cost twice + // the label's width, and a diagram wider than the terminal is word-wrapped + // into fragments, which is worse than the truncation it avoids. let gap_widths: Vec = (0..last_layer) .map(|k| { let exits = feedback.exits[k]; let entries = feedback.entries[k + 1]; - // A bent label is right-aligned against the bend, so it needs one - // column of padding as well; without it the text butts against the - // node border and reads as part of the box. - let bent = match label_room[k] { + let labelled = match label_room[k] { 0 => 0, - label => 2 * (label + exits + 1), + label => label + exits + entries + 4, }; node_h_gap .max(exits + entries + 4) .max(2 * exits) .max(2 * entries + 3) - .max(bent) - .max(label_room[k] + entries + 2) + .max(labelled) }) .collect(); // Routes into the first column or out of the last one use the margins. @@ -2033,17 +2036,40 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz } } + // Bend column of each gap. Every forward edge between two adjacent columns + // turns here whatever the widths of the individual nodes, so their runs + // converge into one junction rather than fanning out. It sits at the + // middle of the gap, except that a gap carrying a label has to fit that + // label between its drop columns and the bend, which pushes the bend right + // by as much as the label needs and no further. `gap_widths` above is + // sized for exactly this placement, so the `min` never binds; it is there + // so that a future change to one of the two cannot silently walk the bend + // into the rise columns. + let bend_x: Vec = (0..last_layer) + .map(|k| { + let right = col_bounds[k].1; + let left = col_bounds[k + 1].0; + let centred = right + 1 + (left - right - 1) / 2; + let needed = match label_room[k] { + 0 => 0, + label => right + 2 + feedback.exits[k] + label, + }; + centred + .max(needed) + .min(left.saturating_sub(2 + feedback.entries[k + 1])) + }) + .collect(); + // Forward edges for (idx, edge) in graph.edges.iter().enumerate() { if layout.feedback_set.contains(&idx) { continue; } if let (Some(src), Some(dst)) = (positions.get(&edge.from), positions.get(&edge.to)) { - // Bend in the middle of the gap between the two columns, so that - // every edge between them turns in the same column no matter how - // wide the individual nodes are. Between adjacent columns the gap - // budget already keeps that column clear; an edge spanning further - // is nudged right off any column a route holds. + // Between adjacent columns the bend is the gap's own bend column, + // which the budget keeps clear of every drop and rise. An edge + // spanning further has no such column, so it takes the middle of + // its own span and is nudged right off any column a route holds. let (mid_x, label_bounds) = match ( layout.node_pos.get(&edge.from), layout.node_pos.get(&edge.to), @@ -2051,14 +2077,18 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz (Some(&(src_layer, _)), Some(&(dst_layer, _))) => { let src_right = col_bounds[src_layer].1; let dst_left = col_bounds[dst_layer].0; - let mid = (dst_left > src_right + 1) - .then(|| src_right + 1 + (dst_left - src_right - 1) / 2) - .map(|mut x| { - while x + 1 < dst_left && reserved_cols.contains(&x) { - x += 1; - } - x - }); + let mid = if dst_layer == src_layer + 1 { + bend_x.get(src_layer).copied() + } else { + (dst_left > src_right + 1) + .then(|| src_right + 1 + (dst_left - src_right - 1) / 2) + .map(|mut x| { + while x + 1 < dst_left && reserved_cols.contains(&x) { + x += 1; + } + x + }) + }; // Free columns for the label: right of this column's drops, // left of the next column's rises, and clear of the // arrowhead column just left of that column's boxes. An @@ -2341,14 +2371,14 @@ mod tests { "graph LR\n A[Start] --> B{Decision}\n B -->|Yes| C[Action 1]\n B -->|No| D[Do]\n C --> E[End]\n D --> E\n", r#" - ┌──────────┐ - ┌──▶│ Action 1 │───┐ - ┌───────┐ ◆────────────◆ Yes│ └──────────┘ │ ┌─────┐ - │ Start │─────▶│ Decision │────┤ ├─▶│ End │ - └───────┘ ◆────────────◆ No│ │ └─────┘ - │ ┌─────┐ │ - └─────▶│ Do │─────┘ - └─────┘ + ┌──────────┐ + ┌─▶│ Action 1 │───┐ + ┌───────┐ ◆────────────◆ Yes│ └──────────┘ │ ┌─────┐ + │ Start │─────▶│ Decision │────┤ ├─▶│ End │ + └───────┘ ◆────────────◆ No│ │ └─────┘ + │ ┌─────┐ │ + └────▶│ Do │─────┘ + └─────┘ "#, ); @@ -2916,17 +2946,17 @@ mod tests { "graph LR\n A --> B\n A -->|no| C\n B --> D\n C --> D\n D --> B\n", r#" - ┌─────┐ - ┌─▶│ B │───┐ - ┌─────┐ │ └─────┘ │ ┌─────┐ - │ A │───┤ ▲ ├─▶│ D │ - └─────┘ no│┌──┘ │ └─────┘ - ││ ┌─────┐ │ └─┐ - └┼▶│ C │───┘ │ - │ └─────┘ │ - │ │ - │ │ - └─────────────────────┘ + ┌─────┐ + ┌──▶│ B │───┐ + ┌─────┐ │ └─────┘ │ ┌─────┐ + │ A │───┤ ▲ ├─▶│ D │ + └─────┘ no│ ┌──┘ │ └─────┘ + │ │ ┌─────┐ │ └─┐ + └─┼▶│ C │───┘ │ + │ └─────┘ │ + │ │ + │ │ + └─────────────────────┘ "#, ); @@ -2940,14 +2970,14 @@ mod tests { "graph LR\n S --> X\n S --> Y\n X --> S\n Y --> S\n X --> Z1\n Y -->|lbl| Z2\n", r#" - ┌─────┐ ┌─────┐ - ┌─▶│ X │───────────▶│ Z1 │ - ┌─────┐ │ └─────┘ └─────┘ + ┌─────┐ ┌─────┐ + ┌─▶│ X │────────▶│ Z1 │ + ┌─────┐ │ └─────┘ └─────┘ │ S │───┤ └──┐ └─────┘ │ │ - ▲ │ ┌─────┐ │lbl ┌─────┐ -┌──┘ └─▶│ Y │─┼─────────▶│ Z2 │ -│ └─────┘ │ └─────┘ + ▲ │ ┌─────┐ │lbl ┌─────┐ +┌──┘ └─▶│ Y │─┼──────▶│ Z2 │ +│ └─────┘ │ └─────┘ │ └─┐│ │ ││ ├─────────────────────┼┘ @@ -2989,15 +3019,71 @@ mod tests { "graph LR\n A[Start] --> B{Check}\n B -->|success| C[Deploy]\n B -->|failure| D[Rollback]\n", r#" - ┌────────┐ - ┌───────▶│ Deploy │ - ┌───────┐ ◆─────────◆ success│ └────────┘ + ┌────────┐ + ┌──▶│ Deploy │ + ┌───────┐ ◆─────────◆ success│ └────────┘ │ Start │─────▶│ Check │────────┤ └───────┘ ◆─────────◆ failure│ - │ ┌──────────┐ - └──────▶│ Rollback │ - └──────────┘ + │ ┌──────────┐ + └─▶│ Rollback │ + └──────────┘ + +"#, + ); + } + + #[test] + fn straight_label_clears_two_feedback_drops_left_right() { + // Y's column holds two feedback sources, so the gap right of it opens + // with two drop columns and the label has to start past both. Sizing + // the gap for the label and the rises alone leaves it a column short + // per drop, and `committed` comes back as `committ…`. + assert_render( + "graph LR\n S --> X\n S --> Y\n X --> S\n Y --> S\n X --> Z1\n Y -->|committed| Z2\n", + r#" + + ┌─────┐ ┌─────┐ + ┌─▶│ X │──────────────▶│ Z1 │ + ┌─────┐ │ └─────┘ └─────┘ + │ S │───┤ └──┐ + └─────┘ │ │ + ▲ │ ┌─────┐ │committed ┌─────┐ +┌──┘ └─▶│ Y │─┼────────────▶│ Z2 │ +│ └─────┘ │ └─────┘ +│ └─┐│ +│ ││ +├─────────────────────┼┘ +│ │ +└─────────────────────┘ +"#, + ); + } + + #[test] + fn a_bent_label_does_not_double_the_gap_left_right() { + // The bend moves right with the label instead of holding the middle of + // the gap. Sizing a centred bend to clear an 18-column label costs 38 + // columns of gap against 22 here, and the viewer word-wraps a diagram + // wider than the terminal into fragments. + let code = "graph LR\n A[Start] --> B{Check}\n B -->|user clicks submit| C[Deploy]\n B -->|failure| D[Rollback]\n D --> B\n"; + let text = render_text(code); + let width = text.lines().map(|l| l.chars().count()).max().unwrap_or(0); + assert!(width < 70, "diagram is {width} columns wide:\n{text}"); + assert_render( + code, + r#" + ┌────────┐ + ┌──▶│ Deploy │ + ┌───────┐ ◆─────────◆ user clicks submit│ └────────┘ + │ Start │─────▶│ Check │───────────────────┤ + └───────┘ ◆─────────◆ failure│ + ▲ │ ┌──────────┐ + ┌──┘ └─▶│ Rollback │ + │ └──────────┘ + │ └─┐ + │ │ + └──────────────────────────────────────────────┘ "#, ); } @@ -3013,14 +3099,14 @@ mod tests { ┌─────┐ │ A │────┐ - └─────┘ lbl│ ┌─────┐ - ├──▶│ X │ - │ └─────┘ + └─────┘ lbl│ ┌─────┐ + ├─▶│ X │ + │ └─────┘ ┌─────┐ │ │ B │────┤ - └─────┘ │ ┌─────┐ - ├──▶│ Y │ - │ └─────┘ + └─────┘ │ ┌─────┐ + ├─▶│ Y │ + │ └─────┘ ┌─────┐ │ │ C │────┘ └─────┘ From 4d4d1c949a4cd7a3acad315f0b92cb8c06878adf Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Tue, 8 Sep 2026 19:02:33 +0530 Subject: [PATCH 09/17] test(diagram): pin how a too-long top-down label gets cut MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A top-down canvas is sized for its boxes and not for its labels, so a long enough label runs out of canvas at the right edge. On main the write simply ran off the end, where `set` dropped it on the floor, and the label stopped mid-word with nothing to say it had been cut. This branch runs it through `fit_label` instead, so it ends in an ellipsis. That is the *only* way an acyclic top-down render differs from main, and up to now nothing said so out loud. The branch description claimed those renders were byte-identical, which was very nearly true and therefore exactly the kind of claim that quietly stops being true. So pin it with a test. No behaviour change here — this is a statement that the ellipsis is deliberate, not a leftover. The asymmetry underneath is real and is not addressed here: left-right diagrams now grow their gaps to fit labels while top-down ones still truncate against a fixed canvas. Making top-down grow too would widen every labelled diagram and cost the byte-identical property that makes this branch reviewable. It can wait for its own change. --- src/diagram.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/diagram.rs b/src/diagram.rs index 3faf331..c024479 100644 --- a/src/diagram.rs +++ b/src/diagram.rs @@ -3115,6 +3115,31 @@ mod tests { ); } + #[test] + fn a_label_overrunning_an_acyclic_canvas_is_cut_with_an_ellipsis_top_down() { + // A top-down canvas is sized for its boxes, not for its labels, so a + // long label runs out of room at the right edge whether or not the + // diagram has feedback edges. `main` let the write run off the canvas, + // where `set` dropped it, and the label ended mid-word with nothing to + // say it had been cut. This is the one way an acyclic top-down render + // differs from `main`. + assert_render( + "graph TD\n A -->|this is an extremely long edge label| B\n", + r#" + ┌─────┐ + │ A │ + └─────┘ + │ this… + │ + │ + ▼ + ┌─────┐ + │ B │ + └─────┘ +"#, + ); + } + #[test] fn a_label_with_no_room_to_be_cut_is_dropped() { assert_eq!(fit_label("retry", 5), "retry"); From 51fef7427906f070b03e855ab6cdba9138e93a20 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Tue, 8 Sep 2026 19:10:15 +0530 Subject: [PATCH 10/17] chore: uncheck three boxes in the sample task list These flipped while poking at the interactive checkboxes from #39 in the viewer. The toggle writes straight back to the file, so a test session leaves footprints in the working tree. Keeping them anyway. The task list in test.md is a rendering sample, not a status report, and a few more unchecked boxes scattered through it exercise the empty-box path better than a wall of green. The features themselves are still very much implemented. --- test.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test.md b/test.md index 6e396fe..1a2fafa 100644 --- a/test.md +++ b/test.md @@ -154,14 +154,14 @@ graph TD - [x] Heading outline / TOC (`o` key) - [x] Follow mode (`--follow`) - [x] Stdin support (`cat file | mdterm`) -- [x] Image placeholders +- [ ] Image placeholders - [x] Heading jumps (`[` / `]`) - [x] Link picker (`f` key) - [x] Copy to clipboard (`y` / `Y` / `c`) - [x] Multiple files (`Tab` / `Shift+Tab`) -- [x] CLI flags (`--help`, `--version`, etc.) +- [ ] CLI flags (`--help`, `--version`, etc.) - [x] Config file (`~/.config/mdterm/config.toml`) -- [x] Line numbers (`l` key or `--line-numbers`) +- [ ] Line numbers (`l` key or `--line-numbers`) - [x] Code block copy (`c` key) - [x] Regex search (`/` with patterns) - [x] Scrollbar From f6914d294583c242c9a17a8ecb245c91f07a1238 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Tue, 8 Sep 2026 20:41:48 +0530 Subject: [PATCH 11/17] test(diagram): don't join a render thread that is still rendering The route/label property test spawns each render on its own thread and waits on a channel with a 20 second timeout, so that a layout which never terminates fails the suite instead of wedging it. That was the whole point of the timeout. Except the timeout arm then called handle.join(). On a timeout the render thread is by definition still running, so joining it waits for exactly the render that just failed to finish. The test hangs instead of failing, and CI sits there until the job limit kills it. The one failure this was built to catch is the one failure it could not report. Detach the thread instead and panic straight away. While at it, split the arm in two: a disconnect means the render panicked, a timeout means it hung, and those are different bugs that deserve different messages. The snapshot helper next door already got this right. --- src/diagram.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/diagram.rs b/src/diagram.rs index c024479..1bc9ebc 100644 --- a/src/diagram.rs +++ b/src/diagram.rs @@ -2275,9 +2275,15 @@ mod tests { match rx.recv_timeout(Duration::from_secs(20)) { Ok(true) => {} Ok(false) => panic!("case {case} did not render:\n{code}"), - Err(err) => { - let _ = handle.join(); - panic!("case {case} failed ({err:?}) for:\n{code}"); + // Neither arm joins the render thread. On a timeout it is + // still running, so waiting for it would hang the test rather + // than fail it, which is the failure this timeout exists to + // report. The thread is detached instead. + Err(mpsc::RecvTimeoutError::Disconnected) => { + panic!("case {case} panicked while rendering:\n{code}") + } + Err(mpsc::RecvTimeoutError::Timeout) => { + panic!("case {case} did not finish within 20 seconds:\n{code}") } } handle.join().unwrap(); From 3e78bc14089b892f853ac099113778c85f47d4e7 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Tue, 8 Sep 2026 20:42:08 +0530 Subject: [PATCH 12/17] fix(diagram): stop labels erasing the head of a feedback edge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A feedback route's arrowhead was drawn with a bare `set`, which writes a character and nothing else. Every other cell of a route goes through `connect_route`, which also marks the cell as feedback and records which way the line runs through it. So the head was the one cell of the route that the invariant machinery could not see. `set_label` refuses to write over a feedback cell and `assert_invariants` reports one that landed on a box, and neither of them knew the arrowhead existed. A label written across it was allowed through without a murmur, and the destination came out looking like a box nothing points at. Mark the head like the rest of the route, and record its direction while at it, so a forward edge crossing it renders as a junction rather than replacing it with a plain line that shows only the forward edge. That alone turns the silence into a failure, and it fires: a random case in the property test really does put a label on an arrowhead. It turns out the reserved rows were short. A route holds every row between the border it leaves and the run it turns onto, and every row between its run and the head it drops to, but only the runs were reserved. A forward edge spanning several layers has no gap budget of its own, so it takes the midpoint of its span and puts its label on the row above — which was free to be the arrowhead row. Reserve the rows a route actually runs through and the collision goes away. Acyclic diagrams reserve nothing, so they are untouched: top-down renders are still byte-identical to main. Three tests: the reduced case from the property run, which keeps seven arrowheads instead of five, and the two shapes nothing pinned before — two back edges leaving one source, and two entering one target. --- src/diagram.rs | 150 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 141 insertions(+), 9 deletions(-) diff --git a/src/diagram.rs b/src/diagram.rs index 1bc9ebc..7c5a52b 100644 --- a/src/diagram.rs +++ b/src/diagram.rs @@ -858,6 +858,31 @@ impl Canvas { } } + /// Draw the arrowhead that ends a feedback route. + /// + /// The head is part of the route and is marked like every other cell of + /// one, so [`Canvas::assert_invariants`] sees it and [`Canvas::set_label`] + /// will not paint over it. `dir` records the direction the route runs + /// through the cell, so a forward edge crossing the head renders as a + /// junction rather than replacing it with a plain line that shows only the + /// forward edge. + fn route_arrow(&mut self, x: usize, y: usize, ch: char, dir: u8, fg: Option) { + if y >= self.height || x >= self.width { + return; + } + let cell = &mut self.cells[y][x]; + // Marked before the node test, not after: a head planned on top of a + // box is the collision `assert_invariants` reports, and painting it + // over the border would hide the very thing being looked for. + cell.is_feedback = true; + if cell.is_node { + return; + } + cell.connects |= dir; + cell.ch = ch; + cell.fg = fg; + } + /// Mark a cell as part of a left-right gutter lane's plain horizontal run, /// the one kind of edge cell that lane's own label may be written over. fn mark_lane(&mut self, x: usize, y: usize) { @@ -896,9 +921,9 @@ impl Canvas { /// /// `add_connection` silently skips a node cell, so a route planned through /// a box does not fail: it renders as a line that stops dead at the border, - /// which reads as an edge the source never declared. `connect_route` marks - /// the cell whether or not the character landed, so the collision is still - /// here to be found afterwards. + /// which reads as an edge the source never declared. `connect_route` and + /// `route_arrow` mark the cell whether or not the character landed, so the + /// collision is still here to be found afterwards. #[cfg(test)] fn assert_invariants(&self) { for (y, row) in self.cells.iter().enumerate() { @@ -1441,7 +1466,7 @@ impl Canvas { for y in (r.entry_y + 1)..arrow_y { self.connect_route(r.entry_x, y, CONN_UP | CONN_DOWN, fg); } - self.set(r.entry_x, arrow_y, '▼', fg); + self.route_arrow(r.entry_x, arrow_y, '▼', CONN_UP | CONN_DOWN, fg); } /// Draw a feedback (back) edge in a left-right diagram. @@ -1496,7 +1521,7 @@ impl Canvas { self.connect_route(x, entry_y, CONN_LEFT | CONN_RIGHT, fg); } self.connect_route(r.entry_x, entry_y, CONN_LEFT | CONN_UP, fg); - self.set(r.entry_x, r.dst_bottom_y + 1, '▲', fg); + self.route_arrow(r.entry_x, r.dst_bottom_y + 1, '▲', CONN_UP | CONN_DOWN, fg); } pub(crate) fn to_span_rows(&self, theme: &Theme) -> Vec> { @@ -1750,13 +1775,26 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz // Every row a feedback route reserved, so that a forward edge whose span // no gap budget covers can be moved off one. + // + // A route holds more rows than the one its horizontal run sits on. Leaving + // a source it drops from the bottom border down to its run, and entering a + // destination it drops from its run to the arrowhead just above the top + // border, so the rows between are its as well. Reserving only the runs left + // the arrowhead row free, and a forward edge spanning several layers put + // its label there and erased the head of the edge arriving. let mut reserved_rows: HashSet = HashSet::new(); for (k, &top) in layer_top.iter().enumerate() { - for r in 0..feedback.exits[k] { - reserved_rows.insert(top + 4 + r); + if feedback.exits[k] > 0 { + // Stem rows, then the run of the outermost rank. + for y in (top + 3)..=(top + 3 + feedback.exits[k]) { + reserved_rows.insert(y); + } } - for r in 0..feedback.entries[k] { - reserved_rows.insert(top.saturating_sub(2 + r)); + if feedback.entries[k] > 0 { + // The run of the outermost rank, then down to the arrowhead. + for y in top.saturating_sub(1 + feedback.entries[k])..=top.saturating_sub(1) { + reserved_rows.insert(y); + } } } @@ -2697,6 +2735,73 @@ mod tests { ); } + #[test] + fn two_feedback_edges_from_one_source_share_one_stem() { + // Both back edges leave C. They are one endpoint, so they share a rank + // and leave on one stem, which splits at a junction into the two lanes + // rather than drawing a second stem over the first. + assert_render( + "graph TD\n A --> B\n B --> C\n C --> A\n C --> B\n", + r#" + ┌─────────┐ + ▼ │ + ┌─────┐ │ + │ A │ │ + └─────┘ │ + │ │ + │ │ + │ │ + │ ┌─────┐ │ + ▼ ▼ │ │ + ┌─────┐ │ │ + │ B │ │ │ + └─────┘ │ │ + │ │ │ + │ │ │ + │ │ │ + ▼ │ │ + ┌─────┐ │ │ + │ C │ │ │ + └─────┘ │ │ + │ │ │ + └─────────┴───┘ +"#, + ); + } + + #[test] + fn two_feedback_edges_into_one_target_share_one_entry() { + // The mirror: both back edges end at A, so they come down one column + // into a single arrowhead instead of two heads on the same border. + assert_render( + "graph TD\n A --> B\n B --> C\n C --> A\n B --> A\n", + r#" + ┌─────┬───┐ + ▼ │ │ + ┌─────┐ │ │ + │ A │ │ │ + └─────┘ │ │ + │ │ │ + │ │ │ + │ │ │ + ▼ │ │ + ┌─────┐ │ │ + │ B │ │ │ + └─────┘ │ │ + │ │ │ │ + └─┼───────┘ │ + │ │ + │ │ + ▼ │ + ┌─────┐ │ + │ C │ │ + └─────┘ │ + │ │ + └─────────────┘ +"#, + ); + } + // ── Labels ── #[test] @@ -3158,6 +3263,33 @@ mod tests { assert_eq!(fit_label("retry", 0), ""); } + #[test] + fn a_forward_label_clears_a_feedback_arrowhead_top_down() { + // Reduced from a case the route/label property test above generates. + // A forward edge spanning several layers takes the midpoint of its own + // span, which no gap budget accounts for, and writes its label on the + // row above. That row is where a feedback route drops from its + // horizontal run to its arrowhead, and the reserved rows used to cover + // the run alone, so the label went over the head of the edge arriving: + // two boxes that nothing appeared to reach. Reserving the rows the + // whole route runs through is what keeps these seven heads; covering + // only the runs leaves five. + let text = render_text( + "graph TD + N8[x] --> N6[xxxxxxxxxxx] + N5[xxxxxx] --> N7[xxxxxxxxxxxxxxxx] + N1[xxxxxx] --> N3[xxxxxxxxxxxxxxxx] + N6[xxxxxxxxxxx] -->|LLLLLL| N5[xxxxxx] + N4[x] --> N8[x] + N9[xxxxxx] -->|LLLLLLL| N1[xxxxxx] + N7[xxxxxxxxxxxxxxxx] -->|LL| N1[xxxxxx] + N3[xxxxxxxxxxxxxxxx] -->|LLL| N5[xxxxxx] + N7[xxxxxxxxxxxxxxxx] --> N7[xxxxxxxxxxxxxxxx] +", + ); + assert_eq!(text.matches('\u{25bc}').count(), 7, "\n{text}"); + } + #[test] fn one_long_label_does_not_widen_every_lane() { let code = "graph TD\n Start --> Check\n Check --> Work\n Work --> Done\n Done -->|x| Start\n Work -->|a very long label| Check\n"; From f1029b719e15cde580a41019c8a4a939e3deeff3 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Tue, 8 Sep 2026 20:42:24 +0530 Subject: [PATCH 13/17] fix(style): clip a rendered diagram instead of word-wrapping it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A diagram is 2D art on a fixed grid. `wrap_lines` treats every line as prose, so a diagram wider than the terminal gets word-wrapped: the overflow of each row lands on a row of its own, interleaved with the rows below it, and what comes out is not a narrower diagram but a shredded one. Boxes and edges from opposite sides of the drawing end up stacked on top of each other. This has always been reachable, but sizing left-right gaps for their labels made it easy — four labelled back edges is enough to blow past eighty columns. Word-wrap is the wrong tool here and there is no right amount of it. Cut the row at the display width and drop the rest. The left of the diagram stays readable and, more to the point, stays aligned with the rows above and below. Clipping rather than leaving the row long is not a detail. The viewer paints a line in full whatever its width, so an over-wide row runs past the border and the scrollbar and shifts every row after it. A long diagram row does not just look bad, it wrecks the frame. The marker rides on the existing code-content metadata rather than in a new variant, so a diagram is still a code block for every other purpose — clicking one still copies the mermaid source. Only diagrams are clipped; code blocks are text and their long lines wrap exactly as they always have. Note this cuts diagrams in the HTML export too, where the page could have scrolled instead. Truncating beats shredding either way, and `--width` is right there. A per-medium wrap policy can come later if anyone actually wants it. --- src/markdown.rs | 30 ++++++++++--- src/style.rs | 110 +++++++++++++++++++++++++++++++++++++++++++++++- src/viewer.rs | 4 +- 3 files changed, 134 insertions(+), 10 deletions(-) diff --git a/src/markdown.rs b/src/markdown.rs index 92daf20..078ae7d 100644 --- a/src/markdown.rs +++ b/src/markdown.rs @@ -323,7 +323,10 @@ impl<'a> Renderer<'a> { }); self.lines.push(Line { spans: top_spans, - meta: LineMeta::CodeContent { block_id }, + meta: LineMeta::CodeContent { + block_id, + diagram: false, + }, }); // Code lines @@ -403,7 +406,10 @@ impl<'a> Renderer<'a> { self.lines.push(Line { spans, - meta: LineMeta::CodeContent { block_id }, + meta: LineMeta::CodeContent { + block_id, + diagram: false, + }, }); } @@ -416,7 +422,10 @@ impl<'a> Renderer<'a> { ..Default::default() }, }], - meta: LineMeta::CodeContent { block_id }, + meta: LineMeta::CodeContent { + block_id, + diagram: false, + }, }); } @@ -461,7 +470,10 @@ impl<'a> Renderer<'a> { }, }, ], - meta: LineMeta::CodeContent { block_id }, + meta: LineMeta::CodeContent { + block_id, + diagram: true, + }, }); // Diagram content rows @@ -507,7 +519,10 @@ impl<'a> Renderer<'a> { self.lines.push(Line { spans, - meta: LineMeta::CodeContent { block_id }, + meta: LineMeta::CodeContent { + block_id, + diagram: true, + }, }); } @@ -520,7 +535,10 @@ impl<'a> Renderer<'a> { ..Default::default() }, }], - meta: LineMeta::CodeContent { block_id }, + meta: LineMeta::CodeContent { + block_id, + diagram: true, + }, }); } diff --git a/src/style.rs b/src/style.rs index e76517a..96f0d0a 100644 --- a/src/style.rs +++ b/src/style.rs @@ -32,6 +32,11 @@ pub enum LineMeta { }, CodeContent { block_id: usize, + /// A row of a rendered diagram rather than of a code block. A diagram + /// is 2D art on a fixed grid: word-wrapping a row moves its overflow + /// onto a row of its own, where it reads as a second broken diagram + /// instead of a continuation, so an over-wide row is clipped instead. + diagram: bool, }, ListItem { list_id: usize, @@ -94,6 +99,8 @@ pub fn wrap_lines(lines: &[Line], width: usize) -> Vec { for line in lines { if line.spans.is_empty() || line.display_width() <= width { result.push(line.clone()); + } else if matches!(line.meta, LineMeta::CodeContent { diagram: true, .. }) { + result.push(clip_line(line, width)); } else if line .spans .first() @@ -144,6 +151,48 @@ pub fn wrap_lines(lines: &[Line], width: usize) -> Vec { result } +/// Cut `line` down to `width` display columns, dropping the overflow. +/// +/// Used for rows that are laid out on a fixed grid rather than written as +/// prose. Wrapping such a row would reflow it into a shape that no longer +/// lines up with the rows above and below it, and the viewer paints a line in +/// full whatever its width, so leaving it long would run it past the border +/// and shift every row after it. +fn clip_line(line: &Line, width: usize) -> Line { + let mut spans: Vec = Vec::new(); + let mut col = 0; + for span in &line.spans { + let span_width = UnicodeWidthStr::width(span.text.as_str()); + if col + span_width <= width { + col += span_width; + spans.push(span.clone()); + continue; + } + // The span that straddles the edge is cut at a character boundary and + // everything past it is dropped. + let mut text = String::new(); + for ch in span.text.chars() { + let ch_width = unicode_width::UnicodeWidthChar::width(ch).unwrap_or(0); + if col + ch_width > width { + break; + } + col += ch_width; + text.push(ch); + } + if !text.is_empty() { + spans.push(StyledSpan { + text, + style: span.style.clone(), + }); + } + break; + } + Line { + spans, + meta: line.meta.clone(), + } +} + fn word_wrap(line: &Line, width: usize) -> Vec { let mut segments: Vec = Vec::new(); for span in &line.spans { @@ -349,18 +398,75 @@ mod tests { #[test] fn code_meta_propagated_to_first_wrapped_line_only() { let mut line = plain_line("some very long code line content here"); - line.meta = LineMeta::CodeContent { block_id: 5 }; + line.meta = LineMeta::CodeContent { + block_id: 5, + diagram: false, + }; let wrapped = wrap_lines(&[line], 10); assert!(wrapped.len() >= 2); assert!(matches!( wrapped[0].meta, - LineMeta::CodeContent { block_id: 5 } + LineMeta::CodeContent { block_id: 5, .. } )); for l in &wrapped[1..] { assert!(matches!(l.meta, LineMeta::None)); } } + #[test] + fn diagram_row_is_clipped_rather_than_wrapped() { + let mut line = plain_line("┌────────┐ and a tail that does not fit"); + line.meta = LineMeta::CodeContent { + block_id: 1, + diagram: true, + }; + let wrapped = wrap_lines(&[line], 10); + assert_eq!(wrapped.len(), 1, "a diagram row must stay on one row"); + assert_eq!(line_text(&wrapped[0]), "┌────────┐"); + } + + #[test] + fn clipped_diagram_row_keeps_its_meta() { + let mut line = plain_line("┌────────┐ and a tail that does not fit"); + line.meta = LineMeta::CodeContent { + block_id: 7, + diagram: true, + }; + let wrapped = wrap_lines(&[line], 10); + assert!(matches!( + wrapped[0].meta, + LineMeta::CodeContent { + block_id: 7, + diagram: true + } + )); + } + + #[test] + fn diagram_row_that_fits_is_untouched() { + let mut line = plain_line("┌──────┐"); + line.meta = LineMeta::CodeContent { + block_id: 1, + diagram: true, + }; + let wrapped = wrap_lines(&[line], 40); + assert_eq!(wrapped.len(), 1); + assert_eq!(line_text(&wrapped[0]), "┌──────┐"); + } + + #[test] + fn code_rows_still_wrap() { + // Only diagrams are clipped. A code block is text, and its long lines + // wrap as they always did. + let mut line = plain_line("some very long code line content here"); + line.meta = LineMeta::CodeContent { + block_id: 2, + diagram: false, + }; + let wrapped = wrap_lines(&[line], 10); + assert!(wrapped.len() >= 2); + } + #[test] fn exact_width_line_not_wrapped() { let lines = vec![plain_line("12345")]; diff --git a/src/viewer.rs b/src/viewer.rs index 43bcc3f..5e022b4 100644 --- a/src/viewer.rs +++ b/src/viewer.rs @@ -834,7 +834,7 @@ impl ViewerState { for delta in 0..self.viewport() { for &idx in &[line_idx.wrapping_sub(delta), line_idx + delta] { if let Some(line) = self.wrapped.get(idx) - && let LineMeta::CodeContent { block_id } = line.meta + && let LineMeta::CodeContent { block_id, .. } = line.meta { return Some(block_id); } @@ -1188,7 +1188,7 @@ fn handle_event(state: &mut ViewerState, ev: Event) -> bool { && let Some(line) = state.wrapped.get(line_idx) { match line.meta { - LineMeta::CodeContent { block_id } => { + LineMeta::CodeContent { block_id, .. } => { if let Some(block) = state.doc_info.code_blocks.get(block_id) && copy_to_clipboard(&block.content).is_ok() { From ff6d630a0c2d6ef7348adbad88bf053d7cec2bad Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Thu, 10 Sep 2026 16:44:33 +0530 Subject: [PATCH 14/17] fix(diagram): search both ways for a spanning edge's bus row A forward edge spanning more than one layer has no gap budgeted for it, so it takes the midpoint of its own span and then walks off any row a feedback route holds. The walk only ever went *down*. Down has a floor. The rows a layer's feedback entries reserve run unbroken from the outermost entry's own row to the arrowhead row just above the boxes, so a midpoint landing inside that block has nothing free below it. The search hit the floor and returned the arrowhead row anyway, which put the bus straight across the head of every edge arriving at that layer and the label on an entry's run. Nine feedback targets in one layer is enough to get there, eight is fine, and seven arrowheads disappear when you do. So search upward when downward runs out. Somewhere to go always exists: a span of more than one layer covers a whole gap, and every gap is budgeted for a bus row with a free label row above it. Acyclic diagrams reserve nothing, the first candidate is still the midpoint, and top-down renders stay byte-identical to main. The property test could not have found this. It caps a case at 15 nodes and 30 edges, and nine targets carrying their own descendants and back edges needs twenty of each. The new test builds that shape directly and counts arrowheads, because nothing else objects when a bus runs over a head: the junction it leaves behind is a perfectly legitimate character in a perfectly legitimate place. --- src/diagram.rs | 119 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 104 insertions(+), 15 deletions(-) diff --git a/src/diagram.rs b/src/diagram.rs index 7c5a52b..25b9f2d 100644 --- a/src/diagram.rs +++ b/src/diagram.rs @@ -1808,8 +1808,8 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz // and above its entry rows, so neither it nor the label on the row // above it can land on a feedback route. An edge spanning more // than one layer keeps the midpoint of its own span, which no gap - // budget accounts for, so it is pushed down off any reserved row - // it or its label would otherwise land on. + // budget accounts for, so it is moved off any reserved row it or + // its label would otherwise land on. let bus_y = match ( layout.node_pos.get(&edge.from), layout.node_pos.get(&edge.to), @@ -1817,14 +1817,27 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz (Some(&(from, _)), Some(&(to, _))) if to == from + 1 => { Some(src.bottom_y() + 3 + feedback.exits[from]) } - _ if dst.top_y > src.bottom_y() + 1 => { - let mut y = src.bottom_y() + 1 + (dst.top_y - src.bottom_y() - 1) / 2; - while y + 1 < dst.top_y - && (reserved_rows.contains(&y) || reserved_rows.contains(&(y - 1))) - { - y += 1; - } - Some(y) + _ if dst.top_y > src.bottom_y() + 2 => { + // Search down from the midpoint, then back up. Downward + // alone has a floor, and the rows just above it are the + // ones a destination's feedback entries hold: they run + // unbroken from the outermost entry's own row down to the + // arrowhead row. A midpoint landing inside that block has + // nothing free below it, and stopping at the floor put the + // bus across the heads of the arriving edges and the label + // on an entry's horizontal run. + // + // Somewhere to go always exists: a span of more than one + // layer covers a whole gap, and every gap is budgeted for + // a bus row with a label row above it that no route uses. + let lo = src.bottom_y() + 2; + let mid = (src.bottom_y() + 1 + (dst.top_y - src.bottom_y() - 1) / 2) + .clamp(lo, dst.top_y - 1); + let free = + |y: &usize| !reserved_rows.contains(y) && !reserved_rows.contains(&(y - 1)); + let below = (mid..dst.top_y).find(free); + let above = (lo..mid).rev().find(free); + Some(below.or(above).unwrap_or(mid)) } _ => None, }; @@ -2198,18 +2211,24 @@ mod tests { /// Render on a helper thread so that a layout that never terminates fails /// the test instead of hanging the whole test binary. - fn render_text_with_timeout(code: &'static str) -> String { + fn render_text_with_timeout(code: impl Into) -> String { + let code = code.into(); + let source = code.clone(); let (tx, rx) = mpsc::channel(); thread::spawn(move || { - let _ = tx.send(render_text(code)); + let _ = tx.send(render_text(&code)); }); match rx.recv_timeout(Duration::from_secs(10)) { Ok(text) => text, // The sender is dropped as soon as the render thread unwinds, so a - // panic there arrives here as a disconnect, not as a timeout. - Err(mpsc::RecvTimeoutError::Disconnected) => panic!("the render thread panicked"), + // panic there arrives here as a disconnect, not as a timeout. The + // panic itself is on the other thread's output, which says nothing + // about which diagram provoked it, so name it here. + Err(mpsc::RecvTimeoutError::Disconnected) => { + panic!("the render thread panicked on:\n{source}") + } Err(mpsc::RecvTimeoutError::Timeout) => { - panic!("rendering did not finish within 10 seconds") + panic!("rendering did not finish within 10 seconds:\n{source}") } } } @@ -2328,6 +2347,76 @@ mod tests { } } + /// One layer of `targets` nodes, each with its own descendant and its own + /// back edge, optionally with a labelled forward edge spanning two layers + /// into the `label_into`-th of them. + /// + /// The random generator above cannot reach this shape. It caps a case at + /// 15 nodes and 30 edges, and nine targets carrying their own descendants + /// and back edges needs twenty of each. A wide layer is where the row + /// budget is under the most pressure: every feedback target in a layer + /// claims a row of the gap above it, while the midpoint of a forward edge + /// crossing that gap moves by only half a row per row added. + /// + /// `A` shares its layer with `Z` so that it is off the centre line the + /// lone node below it sits on. A spanning edge drawn straight down that + /// centre line runs through the box in between and overwrites the + /// arrowhead entering it, which is a separate and older fault than the one + /// under test here; standing `A` to one side keeps it out of the way. + fn wide_layer_source(dir: &str, targets: usize, label_into: Option) -> String { + let mut code = format!("graph {dir}\n A --> M\n Z --> M\n"); + for i in 1..=targets { + code.push_str(&format!(" M --> T{i}\n")); + code.push_str(&format!(" T{i} --> U{i}\n")); + code.push_str(&format!(" U{i} --> T{i}\n")); + } + if let Some(i) = label_into { + code.push_str(&format!(" A -->|lbl| T{i}\n")); + } + code + } + + /// Every render checks [`Canvas::assert_invariants`] and every label goes + /// through [`Canvas::set_label`], so rendering a shape is itself one + /// assertion: a route laid over a box or a label laid over a route fails + /// here rather than coming out as a break in the picture. + /// + /// Arrowheads are the other. Nothing objects when a forward edge's own + /// horizontal run crosses a feedback arrowhead — `add_connection` turns + /// the head into a junction, which is a legitimate character in a + /// legitimate place — so the only way to see that the head is gone is to + /// count the heads. Every edge here ends at a cell of its own, bar the + /// spanning edge that shares a destination border with `M -> T`, so the + /// count is fixed by the shape: one head per node reached, plus one per + /// back edge. + #[test] + fn a_wide_layer_of_feedback_targets_keeps_every_route_clear() { + for targets in 2..=12usize { + for dir in ["TD", "LR"] { + for label_into in [None, Some(1), Some(targets.div_ceil(2)), Some(targets)] { + let code = wide_layer_source(dir, targets, label_into); + let text = render_text_with_timeout(code.clone()); + // One head at M, one at every T and one at every U. The + // spanning edge adds none: it shares a destination border + // with `M -> T`. Every back edge then adds one of its own, + // entering T through a border the forward edges do not use. + let forward = 2 * targets + 1; + let back = targets; + if dir == "TD" { + // Both kinds of head are the same glyph here. + let heads = text.matches('\u{25bc}').count(); + assert_eq!(heads, forward + back, "\n{code}\n{text}"); + } else { + let out = text.matches('\u{25b6}').count(); + let into = text.matches('\u{25b2}').count(); + assert_eq!(out, forward, "\n{code}\n{text}"); + assert_eq!(into, back, "\n{code}\n{text}"); + } + } + } + } + } + // ── Cycle classification ── #[test] From d853203b4d9a020a3fbb5e247d7c075d9325f2c1 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Thu, 10 Sep 2026 16:44:38 +0530 Subject: [PATCH 15/17] chore: restore the sample task list to its state on main Three checkboxes in test.md got unchecked by poking at the interactive checkboxes in the viewer, which writes straight back to the file. That has nothing to do with routing feedback edges, and it has been sitting in the middle of a 2500-line diff making reviewers ask what it is doing there. Reverting the change rather than dropping the commit that made it: the pull request description references the SHAs of the three commits that follow it, and rewriting those to save one line of diff is a bad trade. --- test.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test.md b/test.md index 1a2fafa..6e396fe 100644 --- a/test.md +++ b/test.md @@ -154,14 +154,14 @@ graph TD - [x] Heading outline / TOC (`o` key) - [x] Follow mode (`--follow`) - [x] Stdin support (`cat file | mdterm`) -- [ ] Image placeholders +- [x] Image placeholders - [x] Heading jumps (`[` / `]`) - [x] Link picker (`f` key) - [x] Copy to clipboard (`y` / `Y` / `c`) - [x] Multiple files (`Tab` / `Shift+Tab`) -- [ ] CLI flags (`--help`, `--version`, etc.) +- [x] CLI flags (`--help`, `--version`, etc.) - [x] Config file (`~/.config/mdterm/config.toml`) -- [ ] Line numbers (`l` key or `--line-numbers`) +- [x] Line numbers (`l` key or `--line-numbers`) - [x] Code block copy (`c` key) - [x] Regex search (`/` with patterns) - [x] Scrollbar From 41af38d69c1561bea8df18c5f34c6b6de34b2f5d Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Thu, 10 Sep 2026 18:06:12 +0530 Subject: [PATCH 16/17] refactor(diagram): stop advertising the router to the rest of the crate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Half of this module is marked pub(crate), and almost none of it needs to be. The JSON card view reaches in for exactly four things — Canvas::new, draw_card, draw_edge_lr and to_span_rows — plus the CardDrawRow it fills in. Everything else is diagram-internal: the feedback route geometry, the cell flags, the connection bits, the layout types, the label fitting. Crate visibility on all of that costs nothing at runtime, but it does mean the compiler cannot tell me when one of them stops being used, and it quietly invites the next person to drive the router from json.rs instead of going through a drawing call. Make the rest private. No behaviour change; the tests live in the same module and never noticed. --- src/diagram.rs | 86 +++++++++++++++++++++++++------------------------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/src/diagram.rs b/src/diagram.rs index 25b9f2d..94bea4c 100644 --- a/src/diagram.rs +++ b/src/diagram.rs @@ -12,7 +12,7 @@ enum Direction { } #[derive(Debug, Clone, Copy, PartialEq)] -pub(crate) enum NodeShape { +enum NodeShape { Rectangle, Rounded, Diamond, @@ -270,10 +270,10 @@ fn parse_arrow(s: &str) -> Option<(Option, &str)> { // ───── Layout ───── #[derive(Clone)] -pub(crate) struct NodeLayout { - pub(crate) center_x: usize, - pub(crate) top_y: usize, - pub(crate) width: usize, +struct NodeLayout { + center_x: usize, + top_y: usize, + width: usize, } impl NodeLayout { @@ -432,45 +432,45 @@ fn plan_feedback(graph: &Graph, layout: &Layout) -> FeedbackPlans { /// Geometry of one feedback edge route in a top-down diagram. /// See [`Canvas::draw_feedback_edge_td`]. -pub(crate) struct FeedbackRouteTd { +struct FeedbackRouteTd { /// Column where the route leaves the source's bottom border. - pub(crate) exit_x: usize, + exit_x: usize, /// Row of the source's bottom border. - pub(crate) src_bottom_y: usize, + src_bottom_y: usize, /// Gap row of the horizontal run below the source. - pub(crate) exit_y: usize, + exit_y: usize, /// Column where the arrowhead enters the destination's top border. - pub(crate) entry_x: usize, + entry_x: usize, /// Row of the destination's top border. - pub(crate) dst_top_y: usize, + dst_top_y: usize, /// Gap row of the horizontal run above the destination. - pub(crate) entry_y: usize, + entry_y: usize, /// Column of the vertical lane in the gutter right of the diagram. - pub(crate) lane_x: usize, + lane_x: usize, } /// Geometry of one feedback edge route in a left-right diagram. /// See [`Canvas::draw_feedback_edge_lr`]. -pub(crate) struct FeedbackRouteLr { +struct FeedbackRouteLr { /// Column where the route leaves the source's bottom border. - pub(crate) exit_x: usize, + exit_x: usize, /// Row of the source's bottom border. - pub(crate) src_bottom_y: usize, + src_bottom_y: usize, /// Gap column right of the source's *column* that carries the drop to the /// lane. Boxes are centred in a column sized by its widest node, so a /// column edge is the only place guaranteed to be clear of every box. - pub(crate) exit_lane_x: usize, + exit_lane_x: usize, /// Column where the arrowhead enters the destination's bottom border. - pub(crate) entry_x: usize, + entry_x: usize, /// Row of the destination's bottom border. - pub(crate) dst_bottom_y: usize, + dst_bottom_y: usize, /// Gap column left of the destination's column that carries the rise from /// the lane, two columns clear of it rather than one: the column /// immediately left of a box is where every forward arrowhead into it /// lands, and sharing it would overwrite the junction. - pub(crate) entry_lane_x: usize, + entry_lane_x: usize, /// Row of the horizontal lane in the gutter below the diagram. - pub(crate) lane_y: usize, + lane_y: usize, } fn assign_layers(graph: &Graph, feedback_edges: &HashSet) -> Vec> { @@ -711,7 +711,7 @@ fn node_box_width(node: &Node) -> usize { label_box_width(&node.label, node.shape) } -pub(crate) fn label_box_width(label: &str, shape: NodeShape) -> usize { +fn label_box_width(label: &str, shape: NodeShape) -> usize { let label_width = label.chars().count(); let width = match shape { NodeShape::Diamond => label_width + 6, @@ -730,7 +730,7 @@ pub(crate) fn label_box_width(label: &str, shape: NodeShape) -> usize { /// anything: one character and an ellipsis reads as a different word, and a /// bare ellipsis says only that something was dropped. The label is left out /// altogether instead, which at least does not misname the edge. -pub(crate) fn fit_label(label: &str, width: usize) -> String { +fn fit_label(label: &str, width: usize) -> String { if label.chars().count() <= width { return label.to_string(); } @@ -746,12 +746,12 @@ pub(crate) fn fit_label(label: &str, width: usize) -> String { // ───── Canvas ───── -pub(crate) const CONN_UP: u8 = 1; -pub(crate) const CONN_DOWN: u8 = 2; -pub(crate) const CONN_LEFT: u8 = 4; -pub(crate) const CONN_RIGHT: u8 = 8; +const CONN_UP: u8 = 1; +const CONN_DOWN: u8 = 2; +const CONN_LEFT: u8 = 4; +const CONN_RIGHT: u8 = 8; -pub(crate) fn junction_char(connects: u8) -> char { +fn junction_char(connects: u8) -> char { match connects { c if c == CONN_UP | CONN_DOWN => '│', c if c == CONN_LEFT | CONN_RIGHT => '─', @@ -773,12 +773,12 @@ pub(crate) fn junction_char(connects: u8) -> char { } #[derive(Clone)] -pub(crate) struct CanvasCell { - pub(crate) ch: char, - pub(crate) fg: Option, - pub(crate) bg: Option, - pub(crate) is_node: bool, - pub(crate) connects: u8, +struct CanvasCell { + ch: char, + fg: Option, + bg: Option, + is_node: bool, + connects: u8, /// Drawn by a feedback route. is_feedback: bool, /// Part of a left-right gutter lane's plain horizontal run. A lane carries @@ -802,8 +802,8 @@ impl Default for CanvasCell { } pub(crate) struct Canvas { - pub(crate) width: usize, - pub(crate) height: usize, + width: usize, + height: usize, cells: Vec>, } @@ -816,14 +816,14 @@ impl Canvas { } } - pub(crate) fn set(&mut self, x: usize, y: usize, ch: char, fg: Option) { + fn set(&mut self, x: usize, y: usize, ch: char, fg: Option) { if y < self.height && x < self.width { self.cells[y][x].ch = ch; self.cells[y][x].fg = fg; } } - pub(crate) fn set_node(&mut self, x: usize, y: usize, ch: char, fg: Option) { + fn set_node(&mut self, x: usize, y: usize, ch: char, fg: Option) { if y < self.height && x < self.width { self.cells[y][x].ch = ch; self.cells[y][x].fg = fg; @@ -831,7 +831,7 @@ impl Canvas { } } - pub(crate) fn add_connection(&mut self, x: usize, y: usize, dir: u8, fg: Option) { + fn add_connection(&mut self, x: usize, y: usize, dir: u8, fg: Option) { if y < self.height && x < self.width { let cell = &mut self.cells[y][x]; if !cell.is_node { @@ -972,7 +972,7 @@ impl Canvas { } #[allow(clippy::too_many_arguments)] - pub(crate) fn draw_node( + fn draw_node( &mut self, cx: usize, y: usize, @@ -1158,7 +1158,7 @@ impl Canvas { /// right of the diagram on every row, so a label long enough to reach them /// would cut a feedback route; it is truncated instead. #[allow(clippy::too_many_arguments)] - pub(crate) fn draw_edge_td( + fn draw_edge_td( &mut self, src_cx: usize, src_bottom_y: usize, @@ -1438,7 +1438,7 @@ impl Canvas { /// Gap rows never contain nodes, so the route cannot pass through a /// sibling of either endpoint. Forward edges it crosses render as /// junctions. - pub(crate) fn draw_feedback_edge_td(&mut self, route: &FeedbackRouteTd, fg: Option) { + fn draw_feedback_edge_td(&mut self, route: &FeedbackRouteTd, fg: Option) { let r = route; // Leave the source: stem down to the gap row, turn, run right to the lane. @@ -1489,7 +1489,7 @@ impl Canvas { /// not beside its box. Boxes are centred in a column as wide as its widest /// node, so only a column edge is guaranteed clear of every box; a margin /// measured from a narrow box can sit inside a wider neighbour. - pub(crate) fn draw_feedback_edge_lr(&mut self, route: &FeedbackRouteLr, fg: Option) { + fn draw_feedback_edge_lr(&mut self, route: &FeedbackRouteLr, fg: Option) { let r = route; let exit_y = r.src_bottom_y + 1; let entry_y = r.dst_bottom_y + 2; From 89cf7d7994f8fcb09a6e7c488775ab3e140dbe42 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Thu, 10 Sep 2026 18:06:35 +0530 Subject: [PATCH 17/17] fix(diagram): keep lines off the arrowhead of another edge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An arrowhead is a single cell, and it is the only cell of an edge that says which of the two boxes is the destination. Draw anything across it and add_connection helpfully repaints it as a junction — a perfectly legitimate character in a perfectly legitimate place, so nothing objects. The edge simply stops saying which way it runs, and the reader is left with two boxes joined by a line that points at neither. It turns out this is not rare. Of the 2000 graphs the property test generates, 926 lost at least one head. main loses them too, so it is not a regression, but two of the ways to get there are new on this branch. Top-down: a forward edge spanning more than one layer has no gap budgeted for it, so it takes the midpoint of its own span, and the rows a feedback route reserves only cover a layer that back edges *enter* — a layer nothing enters leaves its arrowhead row free for the bus to settle on. Left-right is the mirror: the bend search walked right until it ran out of rise columns and stopped on exactly the column those heads occupy. The fix has two halves. A cell holding a head now records the axis its own edge runs through it, and add_connection records a crossing there without repainting the glyph, so the head survives whatever is drawn over it. That alone takes the 926 to zero. Then both routers keep a spanning edge off an arrowhead row or column to begin with, because a bus that has to break over a row of heads belongs somewhere else and every gap is budgeted for one. Reserving those rows moves no layout. Against main, acyclic top-down output over 400 random graphs differs only where main had painted a head away, plus the overrun-label ellipsis already pinned by a test. No snapshot moved. The property test counts heads now, and a finished top-down render asserts that nothing crosses one at all. Left-right cannot assert that yet: each column is centred in the canvas on its own, so a forward edge running in to its own destination can meet a feedback head belonging to a box in another column entirely. That is the same class as a label landing on an unrelated edge's line — it predates this branch, main has it, and it wants a real placement pass rather than another special case bolted on here. --- src/diagram.rs | 335 +++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 294 insertions(+), 41 deletions(-) diff --git a/src/diagram.rs b/src/diagram.rs index 94bea4c..efaa1ce 100644 --- a/src/diagram.rs +++ b/src/diagram.rs @@ -785,6 +785,19 @@ struct CanvasCell { /// its own label inline, so this is the one kind of edge cell a label may /// be written over. is_lane: bool, + /// Set when the cell holds an edge's arrowhead, to the directions the + /// edge itself runs through the cell. + /// + /// The head is the one cell of an edge that says which of the two nodes it + /// joins is the destination, and it is a single cell, so a line drawn + /// across it does not clutter the edge, it deletes the edge's direction + /// outright. [`Canvas::add_connection`] therefore records a crossing here + /// without repainting the glyph, and + /// [`Canvas::assert_no_crossed_arrowheads`] reports any connection across + /// the head's own axis: the head survives the crossing, but the line doing + /// the crossing is drawn with a break in it and should have been routed + /// elsewhere. + arrow_axis: Option, } impl Default for CanvasCell { @@ -797,6 +810,7 @@ impl Default for CanvasCell { connects: 0, is_feedback: false, is_lane: false, + arrow_axis: None, } } } @@ -820,6 +834,20 @@ impl Canvas { if y < self.height && x < self.width { self.cells[y][x].ch = ch; self.cells[y][x].fg = fg; + // Whatever was here has been replaced, so a cell that held an + // arrowhead no longer does and must not go on being protected as + // though it did. + self.cells[y][x].arrow_axis = None; + } + } + + /// Write the arrowhead that ends a forward edge, marking the cell so that + /// a later crossing cannot paint the head away. `axis` is the direction + /// the edge runs through the cell. See [`CanvasCell::arrow_axis`]. + fn set_arrow(&mut self, x: usize, y: usize, ch: char, axis: u8, fg: Option) { + self.set(x, y, ch, fg); + if y < self.height && x < self.width { + self.cells[y][x].arrow_axis = Some(axis); } } @@ -834,8 +862,16 @@ impl Canvas { fn add_connection(&mut self, x: usize, y: usize, dir: u8, fg: Option) { if y < self.height && x < self.width { let cell = &mut self.cells[y][x]; - if !cell.is_node { - cell.connects |= dir; + if cell.is_node { + return; + } + cell.connects |= dir; + // An arrowhead keeps its glyph and its colour. The direction is + // still recorded, so a route drawn through here later joins up + // with the crossing, but repainting the cell as a junction would + // delete the only mark that says where the edge ends, leaving two + // boxes joined by a line that points at neither. + if cell.arrow_axis.is_none() { cell.ch = junction_char(cell.connects); if fg.is_some() { cell.fg = fg; @@ -862,10 +898,10 @@ impl Canvas { /// /// The head is part of the route and is marked like every other cell of /// one, so [`Canvas::assert_invariants`] sees it and [`Canvas::set_label`] - /// will not paint over it. `dir` records the direction the route runs - /// through the cell, so a forward edge crossing the head renders as a - /// junction rather than replacing it with a plain line that shows only the - /// forward edge. + /// will not paint over it. `dir` is the direction the route runs through + /// the cell, and doubles as the head's own axis, so a line crossing it + /// keeps off the glyph and is reported by + /// [`Canvas::assert_no_crossed_arrowheads`]. fn route_arrow(&mut self, x: usize, y: usize, ch: char, dir: u8, fg: Option) { if y >= self.height || x >= self.width { return; @@ -881,6 +917,7 @@ impl Canvas { cell.connects |= dir; cell.ch = ch; cell.fg = fg; + cell.arrow_axis = Some(dir); } /// Mark a cell as part of a left-right gutter lane's plain horizontal run, @@ -936,6 +973,37 @@ impl Canvas { } } + /// Check, once a top-down render is finished, that no line was drawn + /// across an arrowhead. + /// + /// An arrowhead keeps its glyph against a crossing, so the head itself is + /// never lost; what is lost is a cell of the line that crossed it, and a + /// bus that has to break over a row of heads belongs on another row. Every + /// gap is budgeted for one. + /// + /// Top-down only. In a left-right diagram each column is centred in the + /// canvas on its own, so a box's rows line up with nothing in particular + /// in the column beside it, and a forward edge running in to its own + /// destination can meet a feedback arrowhead belonging to a box in another + /// column. That is the same class as a label landing on an unrelated + /// edge's line, which predates this branch and `main` has too: it wants a + /// placement pass that draws every line before deciding where the rest + /// goes, not a special case here. + #[cfg(test)] + fn assert_no_crossed_arrowheads(&self) { + for (y, row) in self.cells.iter().enumerate() { + for (x, cell) in row.iter().enumerate() { + if let Some(axis) = cell.arrow_axis { + assert_eq!( + cell.connects & !axis, + 0, + "an edge crosses the arrowhead at ({x}, {y})" + ); + } + } + } + } + /// True when the cell carries a plain horizontal run and nothing else, so /// a label may be written over it without hiding a corner, a crossing or /// a box. @@ -1187,7 +1255,7 @@ impl Canvas { self.add_connection(src_cx, y, CONN_UP | CONN_DOWN, edge_fg); } // Arrow replaces last segment - self.set(dst_cx, dst_top_y - 1, '▼', edge_fg); + self.set_arrow(dst_cx, dst_top_y - 1, '▼', CONN_UP | CONN_DOWN, edge_fg); // Place label beside the vertical line if let Some(text) = label { @@ -1236,7 +1304,7 @@ impl Canvas { } // Arrow - self.set(dst_cx, dst_top_y - 1, '▼', edge_fg); + self.set_arrow(dst_cx, dst_top_y - 1, '▼', CONN_UP | CONN_DOWN, edge_fg); // Place label above horizontal segment if let Some(text) = label { @@ -1287,7 +1355,7 @@ impl Canvas { self.add_connection(x, src_cy, CONN_LEFT | CONN_RIGHT, edge_fg); } // Arrow replaces last segment - self.set(dst_left_x - 1, dst_cy, '▶', edge_fg); + self.set_arrow(dst_left_x - 1, dst_cy, '▶', CONN_LEFT | CONN_RIGHT, edge_fg); // Label above the horizontal line, held inside the free columns. if let Some(text) = label { @@ -1341,7 +1409,7 @@ impl Canvas { } // Arrow - self.set(dst_left_x - 1, dst_cy, '▶', edge_fg); + self.set_arrow(dst_left_x - 1, dst_cy, '▶', CONN_LEFT | CONN_RIGHT, edge_fg); // Label beside the vertical segment: right of the bend where // there is room for it, otherwise left of the bend, and cut when @@ -1797,6 +1865,16 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz } } } + // Every layer's arrowhead row as well, feedback or not. The rows above + // cover a layer that feedback edges enter; a layer they do not enter + // leaves its arrowhead row free, and a spanning bus settling there runs + // the length of the diagram across the head of every forward edge + // arriving at that layer. The heads survive it now (see + // `CanvasCell::arrow_axis`), but the bus is drawn with a break at each one + // and reads as several lines rather than one. + for &top in layer_top.iter().skip(1) { + reserved_rows.insert(top - 1); + } // Forward edges for (idx, edge) in graph.edges.iter().enumerate() { @@ -1864,6 +1942,8 @@ fn render_td(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz #[cfg(test)] canvas.assert_invariants(); + #[cfg(test)] + canvas.assert_no_crossed_arrowheads(); let rows = canvas.to_span_rows(theme); Some((rows, canvas_width)) @@ -2085,6 +2165,11 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz for r in 0..feedback.entries[k] { reserved_cols.insert(left.saturating_sub(2 + r)); } + // The column immediately left of a box is where every forward + // arrowhead into that column lands. A bend there runs down the heads + // of all of them, which the rises are already kept clear of for the + // same reason. + reserved_cols.insert(left.saturating_sub(1)); } // Bend column of each gap. Every forward edge between two adjacent columns @@ -2131,14 +2216,22 @@ fn render_lr(graph: &Graph, theme: &Theme) -> Option<(Vec>, usiz let mid = if dst_layer == src_layer + 1 { bend_x.get(src_layer).copied() } else { - (dst_left > src_right + 1) - .then(|| src_right + 1 + (dst_left - src_right - 1) / 2) - .map(|mut x| { - while x + 1 < dst_left && reserved_cols.contains(&x) { - x += 1; - } - x - }) + (dst_left > src_right + 1).then(|| { + // Search right from the midpoint, then back left. + // Rightward alone has a wall: the rise columns of + // the destination run unbroken up to the arrowhead + // column beside its boxes, so a midpoint inside + // that block has nothing free to its right, and + // stopping at the wall put the bend on the very + // column those heads occupy. + let lo = src_right + 1; + let mid = lo + (dst_left - lo) / 2; + let free = |x: &usize| !reserved_cols.contains(x); + (mid..dst_left) + .find(free) + .or_else(|| (lo..mid).rev().find(free)) + .unwrap_or(mid) + }) }; // Free columns for the label: right of this column's drops, // left of the next column's rises, and clear of the @@ -2194,9 +2287,7 @@ mod tests { use std::thread; use std::time::Duration; - fn render_text(code: &str) -> String { - let theme = Theme::dark(); - let (rows, _) = render_mermaid(code, &theme).expect("diagram should render"); + fn rows_to_text(rows: Vec>) -> String { rows.into_iter() .map(|row| { row.into_iter() @@ -2209,6 +2300,61 @@ mod tests { .join("\n") } + fn render_text(code: &str) -> String { + let theme = Theme::dark(); + let (rows, _) = render_mermaid(code, &theme).expect("diagram should render"); + rows_to_text(rows) + } + + /// How many arrowheads a graph's shape calls for, as + /// `(forward, feedback)`. + /// + /// Every edge ends in a head, but edges sharing a destination share the + /// head they end at: forward edges all arrive at one cell of the + /// destination's border, and so, separately, do the back edges. So the + /// count is the number of distinct destinations of each kind, and a head + /// missing from a render is an edge whose direction the reader cannot + /// recover. + fn expected_arrowheads(code: &str) -> (usize, usize) { + let graph = parse_mermaid(code).expect("diagram should parse"); + let feedback = classify_feedback_edges(&graph); + let mut forward: HashSet<&str> = HashSet::new(); + let mut back: HashSet<&str> = HashSet::new(); + for (idx, edge) in graph.edges.iter().enumerate() { + if feedback.contains(&idx) { + back.insert(edge.to.as_str()); + } else { + forward.insert(edge.to.as_str()); + } + } + (forward.len(), back.len()) + } + + /// Check a render against the heads its graph calls for. Top-down draws + /// both kinds with the same glyph, so there the two are checked as one + /// total. + fn assert_arrowheads(code: &str, text: &str, lr: bool) { + let (forward, back) = expected_arrowheads(code); + if lr { + assert_eq!( + text.matches('\u{25b6}').count(), + forward, + "forward arrowheads\n{code}\n{text}" + ); + assert_eq!( + text.matches('\u{25b2}').count(), + back, + "feedback arrowheads\n{code}\n{text}" + ); + } else { + assert_eq!( + text.matches('\u{25bc}').count(), + forward + back, + "arrowheads\n{code}\n{text}" + ); + } + } + /// Render on a helper thread so that a layout that never terminates fails /// the test instead of hanging the whole test binary. fn render_text_with_timeout(code: impl Into) -> String { @@ -2288,15 +2434,23 @@ mod tests { } } - /// Render a deterministic spread of graph shapes and let - /// [`Canvas::assert_invariants`] check every feedback route against every - /// box. + /// Render a deterministic spread of graph shapes and check three things + /// on every one: that no feedback route was laid over a box + /// ([`Canvas::assert_invariants`]), that no label was written over a route + /// ([`Canvas::set_label`]), and that every edge still ends in an arrowhead. /// /// The shapes vary in node count, node width, edge count, direction and /// labelling, because a route only collides with a box that a *differently /// sized* neighbour widened the column for. + /// + /// The head count is the check with the widest reach. Nothing else objects + /// when a line is drawn across an arrowhead: the junction left behind is a + /// legitimate character in a legitimate place, and the only sign that + /// anything is wrong is that an edge no longer says which way it runs. + /// Before arrowhead cells were protected, 926 of these 2000 cases lost at + /// least one head. #[test] - fn feedback_routes_never_run_through_a_node_box() { + fn feedback_routes_and_arrowheads_survive_random_graphs() { // xorshift with a fixed seed, so a failure is always reproducible. let mut state: u64 = 0x2545_F491_4F6C_DD1D; let mut next = move || { @@ -2327,15 +2481,19 @@ mod tests { let (tx, rx) = mpsc::channel(); let handle = thread::spawn(move || { let theme = Theme::dark(); - let _ = tx.send(render_mermaid(&source, &theme).is_some()); + let _ = + tx.send(render_mermaid(&source, &theme).map(|(rows, _)| rows_to_text(rows))); }); match rx.recv_timeout(Duration::from_secs(20)) { - Ok(true) => {} - Ok(false) => panic!("case {case} did not render:\n{code}"), - // Neither arm joins the render thread. On a timeout it is - // still running, so waiting for it would hang the test rather - // than fail it, which is the failure this timeout exists to - // report. The thread is detached instead. + Ok(Some(text)) => { + // Only this arm joins. On a timeout the thread is still + // rendering, so waiting for it would hang the test rather + // than fail it, which is the failure the timeout exists to + // report; those arms leave it detached. + handle.join().unwrap(); + assert_arrowheads(&code, &text, lr); + } + Ok(None) => panic!("case {case} did not render:\n{code}"), Err(mpsc::RecvTimeoutError::Disconnected) => { panic!("case {case} panicked while rendering:\n{code}") } @@ -2343,7 +2501,6 @@ mod tests { panic!("case {case} did not finish within 20 seconds:\n{code}") } } - handle.join().unwrap(); } } @@ -2417,6 +2574,98 @@ mod tests { } } + /// A layer whose arrowhead row a spanning forward edge's bus can land on. + /// + /// `Q` is reached by a source that also feeds `R` two layers down, so the + /// `S -> R` bus crosses the whole gap around `Q`. Every sibling of `S` + /// carries a self-loop, which fills the gap below their layer with + /// feedback rows and walks the bus's midpoint down onto the one row it + /// must not have: the row every arrowhead into `Q` sits on. The rows a + /// feedback route reserves cover a layer that back edges *enter*, and + /// nothing enters `Q`, so before every layer's arrowhead row was reserved + /// the search settled there and drew the bus across those heads. + fn arrowhead_row_source(siblings: usize) -> String { + let mut code = String::from("graph TD\n S --> Q\n Q --> R\n S --> R\n"); + for i in 1..=siblings { + code.push_str(&format!(" P{i} --> Q\n")); + code.push_str(&format!(" P{i} --> P{i}\n")); + } + code + } + + #[test] + fn a_spanning_bus_clears_an_arrowhead_row_top_down() { + for siblings in 1..=8usize { + let code = arrowhead_row_source(siblings); + let text = render_text_with_timeout(code.clone()); + // Heads at Q and R, then one per self-loop. + assert_arrowheads(&code, &text, false); + assert_eq!( + text.matches('\u{25bc}').count(), + 2 + siblings, + "\n{code}\n{text}" + ); + } + } + + #[test] + fn a_spanning_bend_clears_an_arrowhead_column_left_right() { + // The mirror in left-right. `A -> C3` spans two columns, so it bends + // at the middle of its own span rather than at a gap's bend column. + // The middle column holds three feedback targets, whose rise columns + // run unbroken up to the column beside their boxes, and the search + // used to walk right until it ran out of rises and stop on exactly + // that column: the one every forward arrowhead into the column lands + // on. It searches both ways now, and treats an arrowhead column as + // taken. + // + // `A`'s label is what pushes the bend into the block, by widening the + // gap that the midpoint is measured across. + let code = "graph LR + A -->|xxxxxxxxxxxxx| B + A --> C3 + A2 --> B2 + A2 --> B3 + B --> C + B2 --> C2 + B3 --> C3 + C --> B + C2 --> B2 + C3 --> B3 +"; + let text = render_text_with_timeout(code); + assert_arrowheads(code, &text, true); + // Pinned in full, because a head that survives a crossing is not the + // whole of it: the line that crossed it is drawn with a break, and + // only the picture shows that. + assert_render( + code, + r#" + + ┌─────┐ ┌─────┐ + ┌────▶│ B │─────▶│ C │ + ┌─────┐ xxxxxxxxxxxxx│ └─────┘ └─────┘ + │ A │──────────────┤ ▲ └───┐ + └─────┘ │ ┌────┘ │ + │ │ ┌─────┐ ┌─────┐ │ + └─┼──▶│ C3 │─────▶│ B3 │ │ + ┌─────┐ │ └─────┘ └─────┘ │ + │ A2 │──────────────┬─┼───┘▲ └──┐│ + └─────┘ │ │┌───┘ ││ + │ ││ ┌─────┐ ┌─────┐ ││ + └─┼┼─▶│ B2 │─────▶│ C2 │ ││ + ││ └─────┘ └─────┘ ││ + ││ ▲ └─┐││ + ││┌──┘ │││ + │└┼─────────────────────┼┘│ + │ │ │ │ + └─┼─────────────────────┼─┘ + │ │ + └─────────────────────┘ +"#, + ); + } + // ── Cycle classification ── #[test] @@ -3360,11 +3609,14 @@ mod tests { // row above. That row is where a feedback route drops from its // horizontal run to its arrowhead, and the reserved rows used to cover // the run alone, so the label went over the head of the edge arriving: - // two boxes that nothing appeared to reach. Reserving the rows the - // whole route runs through is what keeps these seven heads; covering - // only the runs leaves five. - let text = render_text( - "graph TD + // two boxes that nothing appeared to reach. Covering the rows the + // whole route runs through is what stopped that; before it, this shape + // came out with five heads. + // + // The eighth head is the one a crossing forward run used to repaint as + // a junction, kept now that an arrowhead cell holds its glyph against + // [`Canvas::add_connection`]. + let code = "graph TD N8[x] --> N6[xxxxxxxxxxx] N5[xxxxxx] --> N7[xxxxxxxxxxxxxxxx] N1[xxxxxx] --> N3[xxxxxxxxxxxxxxxx] @@ -3374,9 +3626,10 @@ mod tests { N7[xxxxxxxxxxxxxxxx] -->|LL| N1[xxxxxx] N3[xxxxxxxxxxxxxxxx] -->|LLL| N5[xxxxxx] N7[xxxxxxxxxxxxxxxx] --> N7[xxxxxxxxxxxxxxxx] -", - ); - assert_eq!(text.matches('\u{25bc}').count(), 7, "\n{text}"); +"; + let text = render_text(code); + assert_arrowheads(code, &text, false); + assert_eq!(text.matches('\u{25bc}').count(), 8, "\n{text}"); } #[test]