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.

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

[PR] Convenient spacing configuration #598

Merged
merged 4 commits into from
Jun 8, 2020
Merged

[PR] Convenient spacing configuration #598

merged 4 commits into from
Jun 8, 2020

Conversation

uruuru
Copy link
Contributor

@uruuru uruuru commented May 9, 2020

Efforts in the direction of #105.

The new class ElkSpacings is intended to be used by layout algorithm-integrators to configure a graph with spacing values before the graph is passed to the layout algorithm itself. It supports the spacing options provided by CoreOptions.

The LayeredSpacings class adjusts and extends the functionality of ElkSpacings by what layered offers in terms of additional spacing options (in particular the between layers options).

Furthermore, there's a new layout option in Layered.melk: spacing.baseValue. It can be used to alter the overall "spaciousness" of a drawing. In other words, in can in-/decrease all spacing values. If one desires to fine-tune individual spacing option values, the LayeredSpacings class would have to be used manually as described for ElkSpacings above.

@le-cds I'm interested in your opinion regarding the actual integration into layered's ElkGraphImporter (see the code comment for a rationale).

Documenting the usage of the new classes as part of the website shall be done as part of #335.

@uruuru uruuru requested a review from le-cds May 9, 2020 16:51
@uruuru uruuru changed the title Convenient spacing configuration [PR] Convenient spacing configuration May 9, 2020
@le-cds le-cds added alg-layered Affects the ELK Layered algorithm. core Affects the ELK core. enhancement An improvement to existing functionality. labels May 12, 2020
@le-cds le-cds added this to the Release 0.7.0 milestone May 12, 2020
@le-cds
Copy link
Contributor

le-cds commented May 12, 2020

Wow, that's quite a bit to take in, thanks for all the effort! I find it hard to really evaluate it without using it, but just from going over the code, it does make sense.

Using this also solves our problems with layout option defaults when it comes to spacings, right?

@uruuru
Copy link
Contributor Author

uruuru commented May 12, 2020

Using this also solves our problems with layout option defaults when it comes to spacings, right?

I'm not sure I know what you mean.

@le-cds
Copy link
Contributor

le-cds commented May 13, 2020

I'm not sure I know what you mean.

I'm basically referring to #104. This mechanism applies concrete spacing values to all elements, so that the node and label placement and size code won't have to rely on wrong, non-algorithm-specific default values?

@uruuru
Copy link
Contributor Author

uruuru commented May 13, 2020

Sounds like it 😲.

I'll see if I can add a test to verify this, though.

@uruuru
Copy link
Contributor Author

uruuru commented May 18, 2020

It does - I added a straightforward test. Note however, that the application of concrete spacing values only happens if the baseValue property is used (and if the configured base value is different from the default value).

We could discuss whether we want to execute this kind of mechanism by default for all options that are used within common layouter code. However, as currently all default values match between core and layered, there's no real need to do so.

@uruuru uruuru self-assigned this May 18, 2020
@le-cds
Copy link
Contributor

le-cds commented Jun 8, 2020

We could discuss whether we want to execute this kind of mechanism by default for all options that are used within common layouter code. However, as currently all default values match between core and layered, there's no real need to do so.

I guess we could defer that to when this really becomes a problem.

@uruuru uruuru merged commit 4a86e41 into master Jun 8, 2020
@uruuru uruuru deleted the uru/spacings branch June 19, 2020 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alg-layered Affects the ELK Layered algorithm. core Affects the ELK core. enhancement An improvement to existing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants