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

Drop dead code #54

Closed
sailingKieler opened this issue Jul 16, 2020 · 3 comments
Closed

Drop dead code #54

sailingKieler opened this issue Jul 16, 2020 · 3 comments
Assignees

Comments

@sailingKieler
Copy link
Member

// Transfer selectability from KLabel to KText to properly handle the selection handlers
// Should only apply if the KText is not configured but KLabel is configured
if (!kText.getProperties().contains(KlighdProperties.NOT_SELECTABLE)
&& getGraphElement().getProperties().contains(KlighdProperties.NOT_SELECTABLE)) {
kText.setProperty(KlighdProperties.NOT_SELECTABLE,
getGraphElement().getProperty(KlighdProperties.NOT_SELECTABLE));
}

@a-sr @NiklasRentzCAU I would like to remove this code, as getGraphElement().getProperties().contains(KlighdProperties.NOT_SELECTABLE) will never be satisfied. getProperties() returns an EMap<IProperty<?>, Object> containing IPropertyToObjectMapImpls. Hence, KlighdProperties.NOT_SELECTABLE will never be "contained".

Also from a conceptual point of view I don't like these kind of built-in magics.
If I remember right, back then I decided leave the freedom of fine-grained control on the selectability of labels and text fields to the diagram synthesis developers on purpose.

Any objections?

@NiklasRentzCAU
Copy link
Member

I only ever used the suppression of the selectability on KTexts and other rendering elements, not on graph element such as labels. As far as I can see this is also never used for labels anywhere in our code, so this could just be removed safely.

@a-sr
Copy link
Member

a-sr commented Jul 16, 2020

I think the idea was that you usually don't need to create a KText for a label and this way you can configure the selectability of the automatically created KText via the label. However, if it does not work, then we probably don't use it, hence you can delete it.

@sailingKieler
Copy link
Member Author

that you usually don't need to create a KText for a label and this way you can configure the selectability of the automatically created KText via the label

Sure. However, I would expect this kind of thing in some convenience API like the KLabelExtensions, not in the runtime.

I'll take care about it together with #43.

@sailingKieler sailingKieler self-assigned this Jul 16, 2020
sailingKieler added a commit that referenced this issue Jul 16, 2020
…manded in #43

* introduced guarding property 'MULTIPLE_KTEXTS_PER_KLABEL' preserving the existing behavior by default if not explicitly set to 'true'
* relaxed handling of label text, may now be specified within the first KText alternatively
* dropped dead code as discussed in #54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants