Near keys for container

[ShupingHe]

#848

now descendants are allowed for contianer with near attribute

the screenshot can be generated by run script:

./ci/e2ereport.sh -test-case=near_keys_for_container

[ShupingHe]

d2/d2layouts/d2near/layout.go

Lines 43 to 52 in 9ab0d0f

	// These shapes skipped core layout, which means they also skipped label placements
	for _, obj := range constantNears {
	if obj.HasOutsideBottomLabel() {
	obj.LabelPosition = go2.Pointer(string(label.OutsideBottomCenter))
	} else if obj.Attributes.Icon != nil {
	obj.LabelPosition = go2.Pointer(string(label.InsideTopCenter))
	} else {
	obj.LabelPosition = go2.Pointer(string(label.InsideMiddleCenter))
	}
	}

@alixander these codes that set labelPosition of near shape should be removed once we allow descendant, but if outside label is allowed, does that mean these containers should also be included during the core layout? we shouldn't let label position affect the main layout

[alixander]

👏 amazing

[alixander]

these codes that set labelPosition of near shape should be removed once we allow descendant, but if outside label is allowed, does that mean these containers should also be included during the core layout? we shouldn't let label position affect the main layout

why should it be removed? we need to place the labels somewhere. maybe we could place it depending on direction, so that it'll never collide with core layout. e.g. the top-x near objects get labels placed on outside_top, the bottom-x objects get labels to outside_bottom, etc

[alixander]

@donglixiaoche

well, i may misunderstood a little bit, but after run core layout for every near container, the label placement of near container will still be skipped? what i thought is now descendant is allowed, so the label may be placed outside, so i need to consider the dimension of label as well when put these near container inside main layout

actually... i think you're right. if you run core layout on the near object, the label placement should happen too.

ohh and so what you're calculating now is this new x, y right? the position considering the label, otherwise, the label might collide with the main diagram?

[ShupingHe]

@donglixiaoche

well, i may misunderstood a little bit, but after run core layout for every near container, the label placement of near container will still be skipped? what i thought is now descendant is allowed, so the label may be placed outside, so i need to consider the dimension of label as well when put these near container inside main layout

actually... i think you're right. if you run core layout on the near object, the label placement should happen too.

ohh and so what you're calculating now is this new x, y right? the position considering the label, otherwise, the label might collide with the main diagram?

yep, that's what i tried to do, but i found there are still some little bugs to fix, working on it, sorry for deleted comments 🤣

[ShupingHe]

@alixander one thing to confirm, for this line

d2/d2layouts/d2near/layout.go

Line 59 in 718daee

tl, br := boundingBox(obj.Graph)

tl will always be (0, 0) right? any exceptions?

[alixander]

@donglixiaoche no. i can't remember what the exceptions were, but it can't be assumed to always be 0, 0

[ShupingHe]

@alixander you can now continue the code review process

[alixander]

can you add this test? label still off. (got it from an issue with the motivating use case for this feature: #1001 )

direction: right

x -> y: {
  style.stroke: green
}

y -> z: {
  style.stroke: red
}

legend: {
  near: bottom-center
  color1: foo {
    shape: text
    style.font-color: green
  }

  color2: bar {
    shape: text
    style.font-color: red
  }
}

[ShupingHe]

@alixander seems like there are some conflicts, what is the correct way to resolve them? how about i just use these files from master branh, and then rerun TA=1 ./ci/test.sh?

yeah exactly, what i do is just accept all conflicts without modifying, and then regenerate after

confilcts resolved by run git merge -x theirs master and then run TA=1 ./ci/test.sh

[alixander]

apologies for how long my reviews have taken @donglixiaoche . this is really good and i'm excited about this. tested locally across all the layout engine and works beautifully

[ShupingHe]

@alixander no worries, actually i'm on vacation right now. and i know i could do better about the details. and i'd love to pick another hard one, do you got one for me?

[alixander]

@donglixiaoche nice! where are you vacationing?

Does this sound interesting? #1107

• One of the biggest improvements in D2 0.3 was that font sizes got "subset", meaning reduced to only include the characters needed for them. but we don't split by styles. so the letter "b" may be used for italic, but we'll include it for regular and bold fonts as well. theoretically this should be another significant decrease in file size if we can further optimize by style.

Or if you want to stick to rendering stuff, this one could result in some big layout improvements: #1031

Feel free to say "pass" and I'm more than happy to find more!

[ShupingHe]

@alixander haha, i'm now in Tibet watching those big views.

and #1107 sounds interesting, want to do some optimizations about font size 🙋🏻‍♂️

[alixander]

Nice! Definitely hope to go someday.

Could you please leave a comment in that issue? Just a "." or anything will do, GitHub won't let me assign otherwise

[alixander]

beautiful! 💯

is this good to go for merging?

[ShupingHe]

beautiful! 💯

is this good to go for merging?

ready to merge if you don't have any other considerations

[alixander]

nope, that's what the approval means =). i just started taking a habit of confirming with the author after ran into an instance where i merged too early (#1091 (comment)). Thanks @donglixiaoche