Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

allow icon for sql_table, class, code/md #1774

Merged
merged 1 commit into from
Dec 12, 2023
Merged

allow icon for sql_table, class, code/md #1774

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

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ci/release/changelogs/next.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#### Features 🚀

- Icons can be added for special objects (sql_table, class, code, markdown, latex). [#1774](https://github.com/terrastruct/d2/pull/1774)

#### Improvements 🧹

#### Bugfixes ⛑️
Expand Down
20 changes: 12 additions & 8 deletions d2graph/d2graph.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1506,18 +1506,22 @@ func (g *Graph) SetDimensions(mtexts []*d2target.MText, ruler *textmeasure.Ruler

// give shapes with icons extra padding to fit their label
if obj.Icon != nil {
labelHeight := float64(labelDims.Height + INNER_LABEL_PADDING)
// Evenly pad enough to fit label above icon
if desiredWidth == 0 {
paddingX += labelHeight
}
if desiredHeight == 0 {
paddingY += labelHeight
switch shapeType {
case shape.TABLE_TYPE, shape.CLASS_TYPE, shape.CODE_TYPE, shape.TEXT_TYPE:
default:
labelHeight := float64(labelDims.Height + INNER_LABEL_PADDING)
// Evenly pad enough to fit label above icon
if desiredWidth == 0 {
paddingX += labelHeight
}
if desiredHeight == 0 {
paddingY += labelHeight
}
}
}
if desiredWidth == 0 {
switch shapeType {
case shape.TABLE_TYPE, shape.CLASS_TYPE, shape.CODE_TYPE, shape.IMAGE_TYPE:
case shape.TABLE_TYPE, shape.CLASS_TYPE, shape.CODE_TYPE:
default:
if obj.Link != nil {
paddingX += 32
Expand Down
2 changes: 2 additions & 0 deletions d2layouts/d2dagrelayout/layout.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,8 @@ func positionLabelsIcons(obj *d2graph.Object) {
obj.LabelPosition = go2.Pointer(label.OutsideTopRight.String())
return
}
} else if obj.SQLTable != nil || obj.Class != nil || obj.Language != "" {
obj.IconPosition = go2.Pointer(label.OutsideTopLeft.String())
} else {
obj.IconPosition = go2.Pointer(label.InsideMiddleCenter.String())
}
Expand Down
3 changes: 3 additions & 0 deletions d2layouts/d2elklayout/layout.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,7 @@ func Layout(ctx context.Context, g *d2graph.Graph, opts *ConfigurableOpts) (err
if hasTop || hasBottom {
padding := parsePadding(elkNodes[obj].LayoutOptions.Padding)
if hasTop {
// TODO I think this fails to account for a potential inner label of container
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gavin-ts there is currently a collision between the icon and label in ELK. I believe it's unrelated to my change and that this code has a bug as described. Can you confirm if so or if I'm just missing something.

Screen Shot 2023-12-11 at 4 16 05 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah it looks like elk hasn't been updated for this. previously all icon positions would be placed in an inside position by default so this would only be possible with custom set positions.

not sure why but elk only seems to be adjusting horizontally for icons but not vertically

d2/d2layouts/d2elklayout/layout.go

Lines 1125 to 1131 in 56a549d

switch position {
case label.OutsideLeftTop, label.OutsideLeftMiddle, label.OutsideLeftBottom,
label.OutsideRightTop, label.OutsideRightMiddle, label.OutsideRightBottom:
width += d2target.MAX_ICON_SIZE + label.PADDING
default:
width = go2.Max(width, d2target.MAX_ICON_SIZE+2*label.PADDING)
}

also noticed elk layout is still doing some old height manipulations but we already look for intersections with outside labels so these are no longer needed

d2/d2layouts/d2elklayout/layout.go

Lines 1184 to 1187 in 56a549d

// special handling to start/end connections below label
if obj.HasOutsideBottomLabel() {
obj.Height -= float64(obj.LabelDimensions.Height) + label.PADDING
}

d2/d2layouts/d2elklayout/layout.go

Lines 1112 to 1115 in 56a549d

// special handling
if obj.HasOutsideBottomLabel() || obj.Icon != nil {
height += float64(obj.LabelDimensions.Height) + label.PADDING
}

d2/d2graph/layout.go

Lines 439 to 445 in 56a549d

if intersections := labelBox.Intersections(startingSegment); len(intersections) > 0 {
overlapsOutsideLabel = true
p := intersections[0]
if len(intersections) > 1 {
p = findOuterIntersection(labelPosition, intersections)
}
// move starting segment to label intersection point

Copy link
Contributor

Choose a reason for hiding this comment

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

in the elk icon_positions test it appears this is unaccounted for prior to your PR, it also may not be accounted for in dagre if the label were inside

Screenshot 2023-12-11 at 5 38 05 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay, i'll get in followup then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gavin-ts
are you sure about the "these are no longer needed"? Many tests change when I remove those:

8ce19c5

Copy link
Collaborator Author

@alixander alixander Dec 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i reverted that file, there's still changes: 705225e

Copy link
Contributor

Choose a reason for hiding this comment

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

8ce19c5#r134821504

looks like we do want the || icon part for this
Screenshot 2023-12-12 at 10 46 19 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we may actually want this adjustment for all outside labels/icons unless we adjust the spacing in another way
8ce19c5#r134822027

to avoid this
Screenshot 2023-12-12 at 10 49 31 AM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is this captured in #1776 or can you make another issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes this is captured in #1776, all margins are now included in the width/height passed to elk, so elk ends up routing the edge distance to shape + margin, instead of routing to shape then cutting at margin.

padding.top = go2.Max(padding.top, d2target.MAX_ICON_SIZE+2*label.PADDING)
}
if hasBottom {
Expand Down Expand Up @@ -1207,6 +1208,8 @@ func positionLabelsIcons(obj *d2graph.Object) {
obj.LabelPosition = go2.Pointer(label.InsideTopRight.String())
return
}
} else if obj.SQLTable != nil || obj.Class != nil || obj.Language != "" {
obj.IconPosition = go2.Pointer(label.OutsideTopLeft.String())
} else {
obj.IconPosition = go2.Pointer(label.InsideMiddleCenter.String())
}
Expand Down
16 changes: 16 additions & 0 deletions d2renderers/d2svg/class.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package d2svg

import (
"fmt"
"html"
"io"

"oss.terrastruct.com/d2/d2target"
Expand Down Expand Up @@ -141,4 +142,19 @@ func drawClass(writer io.Writer, diagramHash string, targetShape d2target.Shape)
)
rowBox.TopLeft.Y += rowHeight
}

if targetShape.Icon != nil && targetShape.Type != d2target.ShapeImage {
iconPosition := label.FromString(targetShape.IconPosition)
iconSize := d2target.GetIconSize(box, targetShape.IconPosition)

tl := iconPosition.GetPointOnBox(box, label.PADDING, float64(iconSize), float64(iconSize))

fmt.Fprintf(writer, `<image href="%s" x="%f" y="%f" width="%d" height="%d" />`,
html.EscapeString(targetShape.Icon.String()),
tl.X,
tl.Y,
iconSize,
iconSize,
)
}
}
16 changes: 16 additions & 0 deletions d2renderers/d2svg/table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package d2svg

import (
"fmt"
"html"
"io"

"oss.terrastruct.com/d2/d2target"
Expand Down Expand Up @@ -163,4 +164,19 @@ func drawTable(writer io.Writer, diagramHash string, targetShape d2target.Shape)
lineEl.Style = "stroke-width:2"
fmt.Fprint(writer, lineEl.Render())
}

if targetShape.Icon != nil && targetShape.Type != d2target.ShapeImage {
iconPosition := label.FromString(targetShape.IconPosition)
iconSize := d2target.GetIconSize(box, targetShape.IconPosition)

tl := iconPosition.GetPointOnBox(box, label.PADDING, float64(iconSize), float64(iconSize))

fmt.Fprintf(writer, `<image href="%s" x="%f" y="%f" width="%d" height="%d" />`,
html.EscapeString(targetShape.Icon.String()),
tl.X,
tl.Y,
iconSize,
iconSize,
)
}
}
2 changes: 1 addition & 1 deletion e2etests-cli/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -999,7 +999,7 @@ func waitLogs(ctx context.Context, buf *bytes.Buffer, pattern *regexp.Regexp) (s
ticker := time.NewTicker(10 * time.Millisecond)
defer ticker.Stop()
var match string
for i := 0; i < 100 && match == ""; i++ {
for i := 0; i < 1000 && match == ""; i++ {
select {
case <-ticker.C:
out := buf.String()
Expand Down
Binary file modified e2etests-cli/testdata/TestCLI_E2E/internal_linked_pdf.exp.pdf
View file
Open in desktop
Binary file not shown.
15 changes: 15 additions & 0 deletions e2etests/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"testing"

"cdr.dev/slog"
"golang.org/x/tools/txtar"

trequire "github.com/stretchr/testify/require"

Expand Down Expand Up @@ -42,6 +43,7 @@ func TestE2E(t *testing.T) {
t.Run("unicode", testUnicode)
t.Run("root", testRoot)
t.Run("themes", testThemes)
t.Run("txtar", testTxtar)
}

func testSanity(t *testing.T) {
Expand Down Expand Up @@ -75,6 +77,19 @@ a -> c
runa(t, tcs)
}

func testTxtar(t *testing.T) {
var tcs []testCase
archive, err := txtar.ParseFile("./testdata/txtar.txt")
assert.Success(t, err)
for _, f := range archive.Files {
tcs = append(tcs, testCase{
name: f.Name,
script: string(f.Data),
})
}
runa(t, tcs)
}

type testCase struct {
name string
// if the test is just testing a render/style thing, no need to exercise both engines
Expand Down
49 changes: 49 additions & 0 deletions e2etests/testdata/txtar.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
-- sql-icon --
without: {
tableEx: {
shape: sql_table
a: b
}
classEx: {
shape: class
a: b
}
codeEx: |go
a := 1
|
mdEx: |md
# This is for all ill-treated fellows

You will live a long, healthy, happy life and make bags of money.
|
}

with: {
tableEx: {
shape: sql_table
a: b
icon: https://icons.terrastruct.com/essentials%2F213-alarm.svg
}
classEx: {
shape: class
a: b
icon: https://icons.terrastruct.com/essentials%2F213-alarm.svg
}
codeEx: |go
a := 1
| {
icon: https://icons.terrastruct.com/essentials%2F213-alarm.svg
}
mdEx: |md
# This is for all ill-treated fellows

You will live a long, healthy, happy life and make bags of money.
| {
icon: https://icons.terrastruct.com/essentials%2F213-alarm.svg
}
}

without.tableEx -> with.tableEx
without.classEx -> with.classEx
without.codeEx -> with.codeEx
without.mdEx -> with.mdEx
Loading