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

Jump to bottom

Replace 'spacing.labelNode' with 'nodeLabels.margin' #576

Open
uruuru opened this issue Apr 22, 2020 · 4 comments · May be fixed by #621
Open

Replace 'spacing.labelNode' with 'nodeLabels.margin' #576

uruuru opened this issue Apr 22, 2020 · 4 comments · May be fixed by #621
Assignees
@soerendomroes @uruuru
Labels
discussion Further discussion required before implementation. question Request for support in using ELK.
Milestone
Release 0.10.0

Comments

@uruuru
Copy link
Contributor

uruuru commented Apr 22, 2020 •
edited
Loading

Consider

// TODO only applied to outside labels?
spacing.labelNode: 30
nodeLabels.padding: "[top=10.0,left=10.0,bottom=10.0,right=10.0]"

node n3 {
	nodeLabels.placement: "H_CENTER V_TOP INSIDE"
	label "label1"
	label "label2"
}

node n4 {
	nodeLabels.placement: "H_CENTER V_TOP OUTSIDE"
	label "label1"
	label "label2"
}

which yields
image

The labelNode is only applied to the OUTSIDE labels, while padding is used on the inside. @le-cds is it possible that labelNode is partly deprecated in this case after your node label placement changes?
Maybe it's reasonable to replace the labelNode by a new margins property for the outside?
@uruuru uruuru added question Request for support in using ELK. discussion Further discussion required before implementation. labels Apr 22, 2020
@uruuru uruuru added this to the Release 0.7.0 milestone Apr 22, 2020
@le-cds
Copy link
Contributor

le-cds commented Apr 24, 2020

Sure, we could replace the label-node spacing by margins, which would be configurable for all four sides independently.

You don't see a bug here, do you? I'd consider this expected behaviour?

@uruuru
Copy link
Contributor Author

uruuru commented Apr 24, 2020

Not a bug but inconsistent behavior I'd say.

If we were introduce nodeLabels.margins, spacing.nodeLabel would be obsolete, wouldn't it? In that case we should

  • a) either remove it or
  • b) use it - if it's set - to fill all values of padding and margin with nodeLabel.

I kind of like b) since that would preserve the possibility to control all spacings using the spacing group.

@le-cds
Copy link
Contributor

le-cds commented Apr 24, 2020

Ah, I see why you find it inconsistent.

If we were introduce nodeLabels.margins, spacing.nodeLabel would be obsolete, wouldn't it?

Absolutely. I guess I'd simply remove it. The spacing options are already way too complex...

uruuru added a commit that referenced this issue May 19, 2020
@uruuru
core: #576 turned 'spacing.labelNode' into 'nodeLabels.margin'
aaf32e1
uruuru added a commit that referenced this issue May 19, 2020
@uruuru
layered: #576 adjustment after 'labelNode' removal
e2bcc04
uruuru added a commit that referenced this issue May 19, 2020
@uruuru
core.nodespacing: #576 adjusted NodeMarginCalculator
2402d99 
Additional changes:
 - retrieve margins using IndividualSpacings
 - retrieve margins locally instead of passing them via parameters
uruuru added a commit that referenced this issue May 19, 2020
@uruuru
core.nodespacing: #576 further adjustments
c105e7b
@le-cds le-cds assigned uruuru and unassigned le-cds Jun 18, 2020
@uruuru
Copy link
Contributor Author

uruuru commented Jun 18, 2020

Feel free to participate in the PR :).

@uruuru uruuru changed the title Spacing labelNode not applied Replace 'spacing.labelNode' with 'nodeLabels.margin' Jun 19, 2020
@le-cds le-cds modified the milestones: Release 0.7.0, Release 0.8.0 Jul 30, 2020
@soerendomroes soerendomroes linked a pull request Mar 15, 2022 that will close this issue
[PR] [WIP] remove 'spacing.labelNode', add 'nodeLabels.margin' #621
Open
@soerendomroes soerendomroes self-assigned this Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@soerendomroes soerendomroes

@uruuru uruuru

Labels
discussion Further discussion required before implementation. question Request for support in using ELK.
Projects
None yet
Milestone
Release 0.10.0
Development

Successfully merging a pull request may close this issue.

[PR] [WIP] remove 'spacing.labelNode', add 'nodeLabels.margin' eclipse/elk
3 participants
@soerendomroes @le-cds @uruuru