From 9e34e91d7df6192e31c6e37fc45e1fdf0376f546 Mon Sep 17 00:00:00 2001 From: Gavin Nishizawa Date: Mon, 13 Nov 2023 18:36:39 -0800 Subject: [PATCH 1/5] fix panic with unrouted edge to grid cell container --- d2layouts/d2layouts.go | 44 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/d2layouts/d2layouts.go b/d2layouts/d2layouts.go index 7a05f2533..ca258f00c 100644 --- a/d2layouts/d2layouts.go +++ b/d2layouts/d2layouts.go @@ -111,7 +111,49 @@ func LayoutNested(ctx context.Context, g *d2graph.Graph, graphInfo GraphInfo, co } InjectNested(g.Root, nestedGraph, false) - g.Edges = append(g.Edges, externalEdges...) + for _, e := range externalEdges { + if e.Src.AbsID() == id || e.Dst.AbsID() == id { + // We need to keep this edge extracted to route after grid layout. + // When we extracted curr(grid cell container) externalEdges=[A, C] and nestedGraph.Edges=[B] + // With includeSelf=true + // ┌grid(g.Root)───────────────────┐ ┌grid(g.Root)───────────────────┐ + // │ ┌────┐ ┌curr───────────┐ │ │ ┌────┐ │ + // │ │ │ │ ┌──┐ ┌──┐ │ │ │ │ │ │ + // │ │ ├──A──►│ │ ├─B─►│ │ │ │ => │ │ │ │ + // │ │ ├──────┼C─┼──┼───►│ │ │ │ │ │ │ │ + // │ │ │ │ └──┘ └──┘ │ │ │ │ │ │ + // │ └────┘ └───────────────┘ │ │ └────┘ │ + // └───────────────────────────────┘ └───────────────────────────────┘ + // + // If we add back all external edges, when we extract curr with includeSelf=false, + // externalEdges would be [C], nestedGraph.Edges=[B], and graph.Edges=[A]. + // This would incorrectly leave A in the graph and it wouldn't be routed after grid layout + // With includeSelf=false + // ┌grid(g.Root)───────────────────┐ ┌grid(g.Root)───────────────────┐ + // │ ┌────┐ ┌curr───────────┐ │ │ ┌────┐ ┌curr───────────┐ │ + // │ │ │ │ ┌──┐ ┌──┐ │ │ │ │ │ │ │ │ + // │ │ ├──A──►│ │ ├─B─►│ │ │ │ => │ │ ├──A──►│ │ │ + // │ │ ├──────┼C─┼──┼───►│ │ │ │ │ │ │ │ │ │ + // │ │ │ │ └──┘ └──┘ │ │ │ │ │ │ │ │ + // │ └────┘ └───────────────┘ │ │ └────┘ └───────────────┘ │ + // └───────────────────────────────┘ └───────────────────────────────┘ + // + // Therefore we only add [B, C] since they will be correctly extracted with includeSelf=false + // as externalEdges=[C] and nestedGraph.Edges=[B]. + // With includeSelf=false + // ┌grid(g.Root)───────────────────┐ ┌grid(g.Root)───────────────────┐ + // │ ┌────┐ ┌curr───────────┐ │ │ ┌────┐ ┌curr───────────┐ │ + // │ │ │ │ ┌──┐ ┌──┐ │ │ │ │ │ │ │ │ + // │ │ │ │ │ ├─B─►│ │ │ │ => │ │ │ │ │ │ + // │ │ ├──────┼C─┼──┼───►│ │ │ │ │ │ │ │ │ │ + // │ │ │ │ └──┘ └──┘ │ │ │ │ │ │ │ │ + // │ └────┘ └───────────────┘ │ │ └────┘ └───────────────┘ │ + // └───────────────────────────────┘ └───────────────────────────────┘ + extractedEdges = append(extractedEdges, e) + } else { + g.Edges = append(g.Edges, e) + } + } restoreOrder() // need to update curr *Object incase layout changed it From bc5cee344f31ba452e1a51ba8991b63b7c912460 Mon Sep 17 00:00:00 2001 From: Gavin Nishizawa Date: Mon, 13 Nov 2023 19:21:24 -0800 Subject: [PATCH 2/5] add grid_cell_container_edge test --- e2etests/regression_test.go | 1 + .../files/grid_cell_container_edge.d2 | 9 + .../dagre/board.exp.json | 210 ++++++++++++++++++ .../dagre/sketch.exp.svg | 104 +++++++++ .../elk/board.exp.json | 210 ++++++++++++++++++ .../elk/sketch.exp.svg | 104 +++++++++ 6 files changed, 638 insertions(+) create mode 100644 e2etests/testdata/files/grid_cell_container_edge.d2 create mode 100644 e2etests/testdata/regression/grid_cell_container_edge/dagre/board.exp.json create mode 100644 e2etests/testdata/regression/grid_cell_container_edge/dagre/sketch.exp.svg create mode 100644 e2etests/testdata/regression/grid_cell_container_edge/elk/board.exp.json create mode 100644 e2etests/testdata/regression/grid_cell_container_edge/elk/sketch.exp.svg diff --git a/e2etests/regression_test.go b/e2etests/regression_test.go index 211272caf..e3f45c15b 100644 --- a/e2etests/regression_test.go +++ b/e2etests/regression_test.go @@ -1050,6 +1050,7 @@ cf many required: { loadFromFile(t, "grid_rows_gap_bug"), loadFromFile(t, "grid_image_label_position"), loadFromFile(t, "glob_dimensions"), + loadFromFile(t, "grid_cell_container_edge"), } runa(t, tcs) diff --git a/e2etests/testdata/files/grid_cell_container_edge.d2 b/e2etests/testdata/files/grid_cell_container_edge.d2 new file mode 100644 index 000000000..fc6107312 --- /dev/null +++ b/e2etests/testdata/files/grid_cell_container_edge.d2 @@ -0,0 +1,9 @@ +grid-rows: 1 +grid-columns: 2 + +box1 + +box2: { + explanation +} +box1 -> box2 diff --git a/e2etests/testdata/regression/grid_cell_container_edge/dagre/board.exp.json b/e2etests/testdata/regression/grid_cell_container_edge/dagre/board.exp.json new file mode 100644 index 000000000..1364fd594 --- /dev/null +++ b/e2etests/testdata/regression/grid_cell_container_edge/dagre/board.exp.json @@ -0,0 +1,210 @@ +{ + "name": "", + "isFolderOnly": false, + "fontFamily": "SourceSansPro", + "shapes": [ + { + "id": "box1", + "type": "rectangle", + "pos": { + "x": 0, + "y": 0 + }, + "width": 79, + "height": 167, + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "borderRadius": 0, + "fill": "B6", + "stroke": "B1", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "box1", + "fontSize": 16, + "fontFamily": "DEFAULT", + "language": "", + "color": "N1", + "italic": false, + "bold": true, + "underline": false, + "labelWidth": 34, + "labelHeight": 21, + "labelPosition": "INSIDE_MIDDLE_CENTER", + "zIndex": 0, + "level": 1 + }, + { + "id": "box2", + "type": "rectangle", + "pos": { + "x": 119, + "y": 41 + }, + "width": 190, + "height": 126, + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "borderRadius": 0, + "fill": "B4", + "stroke": "B1", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "box2", + "fontSize": 28, + "fontFamily": "DEFAULT", + "language": "", + "color": "N1", + "italic": false, + "bold": false, + "underline": false, + "labelWidth": 55, + "labelHeight": 36, + "labelPosition": "OUTSIDE_TOP_CENTER", + "zIndex": 0, + "level": 1 + }, + { + "id": "box2.explanation", + "type": "rectangle", + "pos": { + "x": 149, + "y": 71 + }, + "width": 130, + "height": 66, + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "borderRadius": 0, + "fill": "B5", + "stroke": "B1", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "explanation", + "fontSize": 16, + "fontFamily": "DEFAULT", + "language": "", + "color": "N1", + "italic": false, + "bold": true, + "underline": false, + "labelWidth": 85, + "labelHeight": 21, + "labelPosition": "INSIDE_MIDDLE_CENTER", + "zIndex": 0, + "level": 2 + } + ], + "connections": [ + { + "id": "(box1 -> box2)[0]", + "src": "box1", + "srcArrow": "none", + "dst": "box2", + "dstArrow": "triangle", + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "stroke": "B1", + "borderRadius": 10, + "label": "", + "fontSize": 16, + "fontFamily": "DEFAULT", + "language": "", + "color": "N2", + "italic": true, + "bold": false, + "underline": false, + "labelWidth": 0, + "labelHeight": 0, + "labelPosition": "", + "labelPercentage": 0, + "route": [ + { + "x": 79, + "y": 88 + }, + { + "x": 119, + "y": 93 + } + ], + "animated": false, + "tooltip": "", + "icon": null, + "zIndex": 0 + } + ], + "root": { + "id": "", + "type": "", + "pos": { + "x": 0, + "y": 0 + }, + "width": 0, + "height": 0, + "opacity": 0, + "strokeDash": 0, + "strokeWidth": 0, + "borderRadius": 0, + "fill": "N7", + "stroke": "", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "", + "fontSize": 0, + "fontFamily": "", + "language": "", + "color": "", + "italic": false, + "bold": false, + "underline": false, + "labelWidth": 0, + "labelHeight": 0, + "zIndex": 0, + "level": 0 + } +} diff --git a/e2etests/testdata/regression/grid_cell_container_edge/dagre/sketch.exp.svg b/e2etests/testdata/regression/grid_cell_container_edge/dagre/sketch.exp.svg new file mode 100644 index 000000000..da615a321 --- /dev/null +++ b/e2etests/testdata/regression/grid_cell_container_edge/dagre/sketch.exp.svg @@ -0,0 +1,104 @@ +box1box2explanation + + + + + \ No newline at end of file diff --git a/e2etests/testdata/regression/grid_cell_container_edge/elk/board.exp.json b/e2etests/testdata/regression/grid_cell_container_edge/elk/board.exp.json new file mode 100644 index 000000000..9791ab8db --- /dev/null +++ b/e2etests/testdata/regression/grid_cell_container_edge/elk/board.exp.json @@ -0,0 +1,210 @@ +{ + "name": "", + "isFolderOnly": false, + "fontFamily": "SourceSansPro", + "shapes": [ + { + "id": "box1", + "type": "rectangle", + "pos": { + "x": 0, + "y": 0 + }, + "width": 79, + "height": 166, + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "borderRadius": 0, + "fill": "B6", + "stroke": "B1", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "box1", + "fontSize": 16, + "fontFamily": "DEFAULT", + "language": "", + "color": "N1", + "italic": false, + "bold": true, + "underline": false, + "labelWidth": 34, + "labelHeight": 21, + "labelPosition": "INSIDE_MIDDLE_CENTER", + "zIndex": 0, + "level": 1 + }, + { + "id": "box2", + "type": "rectangle", + "pos": { + "x": 119, + "y": 0 + }, + "width": 230, + "height": 166, + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "borderRadius": 0, + "fill": "B4", + "stroke": "B1", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "box2", + "fontSize": 28, + "fontFamily": "DEFAULT", + "language": "", + "color": "N1", + "italic": false, + "bold": false, + "underline": false, + "labelWidth": 55, + "labelHeight": 36, + "labelPosition": "INSIDE_TOP_CENTER", + "zIndex": 0, + "level": 1 + }, + { + "id": "box2.explanation", + "type": "rectangle", + "pos": { + "x": 169, + "y": 50 + }, + "width": 130, + "height": 66, + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "borderRadius": 0, + "fill": "B5", + "stroke": "B1", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "explanation", + "fontSize": 16, + "fontFamily": "DEFAULT", + "language": "", + "color": "N1", + "italic": false, + "bold": true, + "underline": false, + "labelWidth": 85, + "labelHeight": 21, + "labelPosition": "INSIDE_MIDDLE_CENTER", + "zIndex": 0, + "level": 2 + } + ], + "connections": [ + { + "id": "(box1 -> box2)[0]", + "src": "box1", + "srcArrow": "none", + "dst": "box2", + "dstArrow": "triangle", + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "stroke": "B1", + "borderRadius": 10, + "label": "", + "fontSize": 16, + "fontFamily": "DEFAULT", + "language": "", + "color": "N2", + "italic": true, + "bold": false, + "underline": false, + "labelWidth": 0, + "labelHeight": 0, + "labelPosition": "", + "labelPercentage": 0, + "route": [ + { + "x": 79, + "y": 83 + }, + { + "x": 119, + "y": 83 + } + ], + "animated": false, + "tooltip": "", + "icon": null, + "zIndex": 0 + } + ], + "root": { + "id": "", + "type": "", + "pos": { + "x": 0, + "y": 0 + }, + "width": 0, + "height": 0, + "opacity": 0, + "strokeDash": 0, + "strokeWidth": 0, + "borderRadius": 0, + "fill": "N7", + "stroke": "", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "", + "fontSize": 0, + "fontFamily": "", + "language": "", + "color": "", + "italic": false, + "bold": false, + "underline": false, + "labelWidth": 0, + "labelHeight": 0, + "zIndex": 0, + "level": 0 + } +} diff --git a/e2etests/testdata/regression/grid_cell_container_edge/elk/sketch.exp.svg b/e2etests/testdata/regression/grid_cell_container_edge/elk/sketch.exp.svg new file mode 100644 index 000000000..e5a75b949 --- /dev/null +++ b/e2etests/testdata/regression/grid_cell_container_edge/elk/sketch.exp.svg @@ -0,0 +1,104 @@ +box1box2explanation + + + + + \ No newline at end of file From 7baebe8cf7d4cac480dc3f95551246774dd43d0a Mon Sep 17 00:00:00 2001 From: Gavin Nishizawa Date: Mon, 13 Nov 2023 21:45:54 -0800 Subject: [PATCH 3/5] Revert "add grid_cell_container_edge test" This reverts commit bc5cee344f31ba452e1a51ba8991b63b7c912460. --- e2etests/regression_test.go | 1 - .../files/grid_cell_container_edge.d2 | 9 - .../dagre/board.exp.json | 210 ------------------ .../dagre/sketch.exp.svg | 104 --------- .../elk/board.exp.json | 210 ------------------ .../elk/sketch.exp.svg | 104 --------- 6 files changed, 638 deletions(-) delete mode 100644 e2etests/testdata/files/grid_cell_container_edge.d2 delete mode 100644 e2etests/testdata/regression/grid_cell_container_edge/dagre/board.exp.json delete mode 100644 e2etests/testdata/regression/grid_cell_container_edge/dagre/sketch.exp.svg delete mode 100644 e2etests/testdata/regression/grid_cell_container_edge/elk/board.exp.json delete mode 100644 e2etests/testdata/regression/grid_cell_container_edge/elk/sketch.exp.svg diff --git a/e2etests/regression_test.go b/e2etests/regression_test.go index e3f45c15b..211272caf 100644 --- a/e2etests/regression_test.go +++ b/e2etests/regression_test.go @@ -1050,7 +1050,6 @@ cf many required: { loadFromFile(t, "grid_rows_gap_bug"), loadFromFile(t, "grid_image_label_position"), loadFromFile(t, "glob_dimensions"), - loadFromFile(t, "grid_cell_container_edge"), } runa(t, tcs) diff --git a/e2etests/testdata/files/grid_cell_container_edge.d2 b/e2etests/testdata/files/grid_cell_container_edge.d2 deleted file mode 100644 index fc6107312..000000000 --- a/e2etests/testdata/files/grid_cell_container_edge.d2 +++ /dev/null @@ -1,9 +0,0 @@ -grid-rows: 1 -grid-columns: 2 - -box1 - -box2: { - explanation -} -box1 -> box2 diff --git a/e2etests/testdata/regression/grid_cell_container_edge/dagre/board.exp.json b/e2etests/testdata/regression/grid_cell_container_edge/dagre/board.exp.json deleted file mode 100644 index 1364fd594..000000000 --- a/e2etests/testdata/regression/grid_cell_container_edge/dagre/board.exp.json +++ /dev/null @@ -1,210 +0,0 @@ -{ - "name": "", - "isFolderOnly": false, - "fontFamily": "SourceSansPro", - "shapes": [ - { - "id": "box1", - "type": "rectangle", - "pos": { - "x": 0, - "y": 0 - }, - "width": 79, - "height": 167, - "opacity": 1, - "strokeDash": 0, - "strokeWidth": 2, - "borderRadius": 0, - "fill": "B6", - "stroke": "B1", - "shadow": false, - "3d": false, - "multiple": false, - "double-border": false, - "tooltip": "", - "link": "", - "icon": null, - "iconPosition": "", - "blend": false, - "fields": null, - "methods": null, - "columns": null, - "label": "box1", - "fontSize": 16, - "fontFamily": "DEFAULT", - "language": "", - "color": "N1", - "italic": false, - "bold": true, - "underline": false, - "labelWidth": 34, - "labelHeight": 21, - "labelPosition": "INSIDE_MIDDLE_CENTER", - "zIndex": 0, - "level": 1 - }, - { - "id": "box2", - "type": "rectangle", - "pos": { - "x": 119, - "y": 41 - }, - "width": 190, - "height": 126, - "opacity": 1, - "strokeDash": 0, - "strokeWidth": 2, - "borderRadius": 0, - "fill": "B4", - "stroke": "B1", - "shadow": false, - "3d": false, - "multiple": false, - "double-border": false, - "tooltip": "", - "link": "", - "icon": null, - "iconPosition": "", - "blend": false, - "fields": null, - "methods": null, - "columns": null, - "label": "box2", - "fontSize": 28, - "fontFamily": "DEFAULT", - "language": "", - "color": "N1", - "italic": false, - "bold": false, - "underline": false, - "labelWidth": 55, - "labelHeight": 36, - "labelPosition": "OUTSIDE_TOP_CENTER", - "zIndex": 0, - "level": 1 - }, - { - "id": "box2.explanation", - "type": "rectangle", - "pos": { - "x": 149, - "y": 71 - }, - "width": 130, - "height": 66, - "opacity": 1, - "strokeDash": 0, - "strokeWidth": 2, - "borderRadius": 0, - "fill": "B5", - "stroke": "B1", - "shadow": false, - "3d": false, - "multiple": false, - "double-border": false, - "tooltip": "", - "link": "", - "icon": null, - "iconPosition": "", - "blend": false, - "fields": null, - "methods": null, - "columns": null, - "label": "explanation", - "fontSize": 16, - "fontFamily": "DEFAULT", - "language": "", - "color": "N1", - "italic": false, - "bold": true, - "underline": false, - "labelWidth": 85, - "labelHeight": 21, - "labelPosition": "INSIDE_MIDDLE_CENTER", - "zIndex": 0, - "level": 2 - } - ], - "connections": [ - { - "id": "(box1 -> box2)[0]", - "src": "box1", - "srcArrow": "none", - "dst": "box2", - "dstArrow": "triangle", - "opacity": 1, - "strokeDash": 0, - "strokeWidth": 2, - "stroke": "B1", - "borderRadius": 10, - "label": "", - "fontSize": 16, - "fontFamily": "DEFAULT", - "language": "", - "color": "N2", - "italic": true, - "bold": false, - "underline": false, - "labelWidth": 0, - "labelHeight": 0, - "labelPosition": "", - "labelPercentage": 0, - "route": [ - { - "x": 79, - "y": 88 - }, - { - "x": 119, - "y": 93 - } - ], - "animated": false, - "tooltip": "", - "icon": null, - "zIndex": 0 - } - ], - "root": { - "id": "", - "type": "", - "pos": { - "x": 0, - "y": 0 - }, - "width": 0, - "height": 0, - "opacity": 0, - "strokeDash": 0, - "strokeWidth": 0, - "borderRadius": 0, - "fill": "N7", - "stroke": "", - "shadow": false, - "3d": false, - "multiple": false, - "double-border": false, - "tooltip": "", - "link": "", - "icon": null, - "iconPosition": "", - "blend": false, - "fields": null, - "methods": null, - "columns": null, - "label": "", - "fontSize": 0, - "fontFamily": "", - "language": "", - "color": "", - "italic": false, - "bold": false, - "underline": false, - "labelWidth": 0, - "labelHeight": 0, - "zIndex": 0, - "level": 0 - } -} diff --git a/e2etests/testdata/regression/grid_cell_container_edge/dagre/sketch.exp.svg b/e2etests/testdata/regression/grid_cell_container_edge/dagre/sketch.exp.svg deleted file mode 100644 index da615a321..000000000 --- a/e2etests/testdata/regression/grid_cell_container_edge/dagre/sketch.exp.svg +++ /dev/null @@ -1,104 +0,0 @@ -box1box2explanation - - - - - \ No newline at end of file diff --git a/e2etests/testdata/regression/grid_cell_container_edge/elk/board.exp.json b/e2etests/testdata/regression/grid_cell_container_edge/elk/board.exp.json deleted file mode 100644 index 9791ab8db..000000000 --- a/e2etests/testdata/regression/grid_cell_container_edge/elk/board.exp.json +++ /dev/null @@ -1,210 +0,0 @@ -{ - "name": "", - "isFolderOnly": false, - "fontFamily": "SourceSansPro", - "shapes": [ - { - "id": "box1", - "type": "rectangle", - "pos": { - "x": 0, - "y": 0 - }, - "width": 79, - "height": 166, - "opacity": 1, - "strokeDash": 0, - "strokeWidth": 2, - "borderRadius": 0, - "fill": "B6", - "stroke": "B1", - "shadow": false, - "3d": false, - "multiple": false, - "double-border": false, - "tooltip": "", - "link": "", - "icon": null, - "iconPosition": "", - "blend": false, - "fields": null, - "methods": null, - "columns": null, - "label": "box1", - "fontSize": 16, - "fontFamily": "DEFAULT", - "language": "", - "color": "N1", - "italic": false, - "bold": true, - "underline": false, - "labelWidth": 34, - "labelHeight": 21, - "labelPosition": "INSIDE_MIDDLE_CENTER", - "zIndex": 0, - "level": 1 - }, - { - "id": "box2", - "type": "rectangle", - "pos": { - "x": 119, - "y": 0 - }, - "width": 230, - "height": 166, - "opacity": 1, - "strokeDash": 0, - "strokeWidth": 2, - "borderRadius": 0, - "fill": "B4", - "stroke": "B1", - "shadow": false, - "3d": false, - "multiple": false, - "double-border": false, - "tooltip": "", - "link": "", - "icon": null, - "iconPosition": "", - "blend": false, - "fields": null, - "methods": null, - "columns": null, - "label": "box2", - "fontSize": 28, - "fontFamily": "DEFAULT", - "language": "", - "color": "N1", - "italic": false, - "bold": false, - "underline": false, - "labelWidth": 55, - "labelHeight": 36, - "labelPosition": "INSIDE_TOP_CENTER", - "zIndex": 0, - "level": 1 - }, - { - "id": "box2.explanation", - "type": "rectangle", - "pos": { - "x": 169, - "y": 50 - }, - "width": 130, - "height": 66, - "opacity": 1, - "strokeDash": 0, - "strokeWidth": 2, - "borderRadius": 0, - "fill": "B5", - "stroke": "B1", - "shadow": false, - "3d": false, - "multiple": false, - "double-border": false, - "tooltip": "", - "link": "", - "icon": null, - "iconPosition": "", - "blend": false, - "fields": null, - "methods": null, - "columns": null, - "label": "explanation", - "fontSize": 16, - "fontFamily": "DEFAULT", - "language": "", - "color": "N1", - "italic": false, - "bold": true, - "underline": false, - "labelWidth": 85, - "labelHeight": 21, - "labelPosition": "INSIDE_MIDDLE_CENTER", - "zIndex": 0, - "level": 2 - } - ], - "connections": [ - { - "id": "(box1 -> box2)[0]", - "src": "box1", - "srcArrow": "none", - "dst": "box2", - "dstArrow": "triangle", - "opacity": 1, - "strokeDash": 0, - "strokeWidth": 2, - "stroke": "B1", - "borderRadius": 10, - "label": "", - "fontSize": 16, - "fontFamily": "DEFAULT", - "language": "", - "color": "N2", - "italic": true, - "bold": false, - "underline": false, - "labelWidth": 0, - "labelHeight": 0, - "labelPosition": "", - "labelPercentage": 0, - "route": [ - { - "x": 79, - "y": 83 - }, - { - "x": 119, - "y": 83 - } - ], - "animated": false, - "tooltip": "", - "icon": null, - "zIndex": 0 - } - ], - "root": { - "id": "", - "type": "", - "pos": { - "x": 0, - "y": 0 - }, - "width": 0, - "height": 0, - "opacity": 0, - "strokeDash": 0, - "strokeWidth": 0, - "borderRadius": 0, - "fill": "N7", - "stroke": "", - "shadow": false, - "3d": false, - "multiple": false, - "double-border": false, - "tooltip": "", - "link": "", - "icon": null, - "iconPosition": "", - "blend": false, - "fields": null, - "methods": null, - "columns": null, - "label": "", - "fontSize": 0, - "fontFamily": "", - "language": "", - "color": "", - "italic": false, - "bold": false, - "underline": false, - "labelWidth": 0, - "labelHeight": 0, - "zIndex": 0, - "level": 0 - } -} diff --git a/e2etests/testdata/regression/grid_cell_container_edge/elk/sketch.exp.svg b/e2etests/testdata/regression/grid_cell_container_edge/elk/sketch.exp.svg deleted file mode 100644 index e5a75b949..000000000 --- a/e2etests/testdata/regression/grid_cell_container_edge/elk/sketch.exp.svg +++ /dev/null @@ -1,104 +0,0 @@ -box1box2explanation - - - - - \ No newline at end of file From ecad4e163b9816262ae4696d562fd96c8406bda3 Mon Sep 17 00:00:00 2001 From: Gavin Nishizawa Date: Mon, 13 Nov 2023 21:27:56 -0800 Subject: [PATCH 4/5] actual fix: correct stale object references --- d2layouts/d2layouts.go | 106 ++++++++++++++++++++--------------------- 1 file changed, 52 insertions(+), 54 deletions(-) diff --git a/d2layouts/d2layouts.go b/d2layouts/d2layouts.go index ca258f00c..931ee2a79 100644 --- a/d2layouts/d2layouts.go +++ b/d2layouts/d2layouts.go @@ -103,77 +103,63 @@ func LayoutNested(ctx context.Context, g *d2graph.Graph, graphInfo GraphInfo, co if isGridCellContainer && gi.isDefault() { // if we are in a grid diagram, and our children have descendants // we need to run layout on them first, even if they are not special diagram types + + // First we extract the grid cell container as a nested graph with includeSelf=true + // resulting in externalEdges=[A, C] and nestedGraph.Edges=[B] + // ┌grid(g.Root)───────────────────┐ ┌grid(g.Root)───────────────────┐ + // │ ┌────┐ ┌curr───────────┐ │ │ ┌────┐ │ + // │ │ │ │ ┌──┐ ┌──┐ │ │ │ │ │ │ + // │ │ ├──A──►│ │ ├─B─►│ │ │ │ => │ │ │ │ + // │ │ ├──────┼C─┼──┼───►│ │ │ │ │ │ │ │ + // │ │ │ │ └──┘ └──┘ │ │ │ │ │ │ + // │ └────┘ └───────────────┘ │ │ └────┘ │ + // └───────────────────────────────┘ └───────────────────────────────┘ nestedGraph, externalEdges := ExtractSubgraph(curr, true) + + // Then we layout curr as a nested graph and re-inject it id := curr.AbsID() err := LayoutNested(ctx, nestedGraph, GraphInfo{}, coreLayout) if err != nil { return err } - InjectNested(g.Root, nestedGraph, false) - for _, e := range externalEdges { - if e.Src.AbsID() == id || e.Dst.AbsID() == id { - // We need to keep this edge extracted to route after grid layout. - // When we extracted curr(grid cell container) externalEdges=[A, C] and nestedGraph.Edges=[B] - // With includeSelf=true - // ┌grid(g.Root)───────────────────┐ ┌grid(g.Root)───────────────────┐ - // │ ┌────┐ ┌curr───────────┐ │ │ ┌────┐ │ - // │ │ │ │ ┌──┐ ┌──┐ │ │ │ │ │ │ - // │ │ ├──A──►│ │ ├─B─►│ │ │ │ => │ │ │ │ - // │ │ ├──────┼C─┼──┼───►│ │ │ │ │ │ │ │ - // │ │ │ │ └──┘ └──┘ │ │ │ │ │ │ - // │ └────┘ └───────────────┘ │ │ └────┘ │ - // └───────────────────────────────┘ └───────────────────────────────┘ - // - // If we add back all external edges, when we extract curr with includeSelf=false, - // externalEdges would be [C], nestedGraph.Edges=[B], and graph.Edges=[A]. - // This would incorrectly leave A in the graph and it wouldn't be routed after grid layout - // With includeSelf=false - // ┌grid(g.Root)───────────────────┐ ┌grid(g.Root)───────────────────┐ - // │ ┌────┐ ┌curr───────────┐ │ │ ┌────┐ ┌curr───────────┐ │ - // │ │ │ │ ┌──┐ ┌──┐ │ │ │ │ │ │ │ │ - // │ │ ├──A──►│ │ ├─B─►│ │ │ │ => │ │ ├──A──►│ │ │ - // │ │ ├──────┼C─┼──┼───►│ │ │ │ │ │ │ │ │ │ - // │ │ │ │ └──┘ └──┘ │ │ │ │ │ │ │ │ - // │ └────┘ └───────────────┘ │ │ └────┘ └───────────────┘ │ - // └───────────────────────────────┘ └───────────────────────────────┘ - // - // Therefore we only add [B, C] since they will be correctly extracted with includeSelf=false - // as externalEdges=[C] and nestedGraph.Edges=[B]. - // With includeSelf=false - // ┌grid(g.Root)───────────────────┐ ┌grid(g.Root)───────────────────┐ - // │ ┌────┐ ┌curr───────────┐ │ │ ┌────┐ ┌curr───────────┐ │ - // │ │ │ │ ┌──┐ ┌──┐ │ │ │ │ │ │ │ │ - // │ │ │ │ │ ├─B─►│ │ │ │ => │ │ │ │ │ │ - // │ │ ├──────┼C─┼──┼───►│ │ │ │ │ │ │ │ │ │ - // │ │ │ │ └──┘ └──┘ │ │ │ │ │ │ │ │ - // │ └────┘ └───────────────┘ │ │ └────┘ └───────────────┘ │ - // └───────────────────────────────┘ └───────────────────────────────┘ - extractedEdges = append(extractedEdges, e) - } else { - g.Edges = append(g.Edges, e) - } - } + g.Edges = append(g.Edges, externalEdges...) restoreOrder() - // need to update curr *Object incase layout changed it - var obj *d2graph.Object + // layout can replace Objects so we need to update the references we are holding onto (curr + externalEdges) + idToObj := make(map[string]*d2graph.Object) for _, o := range g.Objects { - if o.AbsID() == id { - obj = o - break + idToObj[o.AbsID()] = o + } + lookup := func(idStr string) (*d2graph.Object, error) { + o, exists := idToObj[idStr] + if !exists { + return nil, fmt.Errorf("could not find object %#v after layout", idStr) } + return o, nil } - if obj == nil { - return fmt.Errorf("could not find object %#v after layout", id) + curr, err = lookup(id) + if err != nil { + return err + } + for _, e := range externalEdges { + src, err := lookup(e.Src.AbsID()) + if err != nil { + return err + } + e.Src = src + dst, err := lookup(e.Dst.AbsID()) + if err != nil { + return err + } + e.Dst = dst } - curr = obj // position nested graph (excluding curr) relative to curr dx := 0 - curr.TopLeft.X dy := 0 - curr.TopLeft.Y for _, o := range nestedGraph.Objects { - if o.AbsID() == curr.AbsID() { + if o == curr { continue } o.TopLeft.X += dx @@ -183,7 +169,19 @@ func LayoutNested(ctx context.Context, g *d2graph.Graph, graphInfo GraphInfo, co e.Move(dx, dy) } - // now we keep the descendants out until after grid layout + // Then after re-injecting everything, we extract curr with includeSelf=false, + // and externalEdges=[C], nestedGraph.Edges=[B], and graph.Edges=[A]. + // This will leave A in the graph to be routed by grid layout, and C will have cross-graph edge routing + // Note: currently grid layout's cell-cell edge routing, and cross-graph edge routing behave the same, + // but these are simple placeholder routings and they may be different in the future + // ┌grid(g.Root)───────────────────┐ ┌grid(g.Root)───────────────────┐ + // │ ┌────┐ ┌curr───────────┐ │ │ ┌────┐ ┌curr───────────┐ │ + // │ │ │ │ ┌──┐ ┌──┐ │ │ │ │ │ │ │ │ + // │ │ ├──A──►│ │ ├─B─►│ │ │ │ => │ │ ├──A──►│ │ │ + // │ │ ├──────┼C─┼──┼───►│ │ │ │ │ │ │ │ │ │ + // │ │ │ │ └──┘ └──┘ │ │ │ │ │ │ │ │ + // │ └────┘ └───────────────┘ │ │ └────┘ └───────────────┘ │ + // └───────────────────────────────┘ └───────────────────────────────┘ nestedGraph, externalEdges = ExtractSubgraph(curr, false) extractedEdges = append(extractedEdges, externalEdges...) From 01248d479b22a521fddf52b9e828ae6f688e9a56 Mon Sep 17 00:00:00 2001 From: Gavin Nishizawa Date: Mon, 13 Nov 2023 21:47:32 -0800 Subject: [PATCH 5/5] changelog --- ci/release/changelogs/next.md | 1 + 1 file changed, 1 insertion(+) diff --git a/ci/release/changelogs/next.md b/ci/release/changelogs/next.md index 406e245d5..440634fee 100644 --- a/ci/release/changelogs/next.md +++ b/ci/release/changelogs/next.md @@ -26,3 +26,4 @@ - Fixes incorrect appendix icon numbering. [#1704](https://github.com/terrastruct/d2/pull/1704) - Fixes crash when using `--watch` and navigating to an invalid board path [#1693](https://github.com/terrastruct/d2/pull/1693) - Fixes edge case where nested edge globs were creating excess shapes [#1713](https://github.com/terrastruct/d2/pull/1713) +- Fixes a panic with a connection to a grid cell that is a container in TALA [#1729](https://github.com/terrastruct/d2/pull/1729)