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

Support of KLabels consisting of multiple KText fields #43

Open
sailingKieler opened this issue Jun 17, 2020 · 9 comments
Open

Support of KLabels consisting of multiple KText fields #43

sailingKieler opened this issue Jun 17, 2020 · 9 comments
Assignees
Labels
enhancement New feature or request

Comments

@sailingKieler
Copy link
Member

In some client project the ability to compose KLabels of several KText element would be extremely valuable. On the one hand those KTexts shall be stylable differently as well as independently selectable or react on clicks with different actions.

Here's an example:
Bildschirmfoto 2020-06-17 um 09 33 54

Caveat: Labels size computations by layout algorithms (like GraphViz dot) will be impaired for such view models. Consequently, the diagram synthesis developer is in charge of configuring things right.

@sailingKieler sailingKieler added the enhancement New feature or request label Jun 17, 2020
@sailingKieler
Copy link
Member Author

@le-cds @NiklasRentzCAU do see any serious objections against dropping the current restriction of allowing at most KText element per KLabel except the incompatibility with Graphviz dot? What about the label management stuff?

@sailingKieler sailingKieler self-assigned this Jul 13, 2020
@NiklasRentzCAU
Copy link
Member

I guess this would violate the general rule of having one active KRendering element for each KGraphElement. The label size estimation in the PlacementUtil.java of in the microlayout expects a single Rendering to represent the element (as all other graph elements as well) and estimates the size there. Also, the rendering in the Sprotty (and I think also in the SWT case) expects a single rendering for the elment, which in the case of a label happens to be a single KText currently.
For the rendering and size estimation side it would be most fitting to allow a generic KRendering, that may consist of maybe an invisible outline with multiple KTexts placed inside, as also done for example for the SCCharts multi-colored texts within renderings (see here). This would allow to keep the size estimation and rendering mostly if not completely the same. I don't, however, know how this affects label management.

@sailingKieler
Copy link
Member Author

I guess this would violate the general rule of having one active KRendering element for each KGraphElement.

Not necessarily. If the multiple KTexts are required, they have to be "packaged" into an invisible KContainerRendering.

The label size estimation in the PlacementUtil.java of in the microlayout expects a single Rendering to represent the element

The currently single KText can already be wrapped by e.g. KRectangles, so this should be fine.

For the rendering and size estimation side it would be most fitting to allow a generic KRendering

This is already given for labels plus the additional check of containing exactly one KText element within the (potentially) compound figure description. In case of no given figure description a default one is applied, of course.

Hence, the change would basically involve dropping the additional check (relaxing to at least one?) , and making sure the/some reasonable label text is added to the ELK graph in case the layouter requires label text explicitly.

@le-cds
Copy link
Member

le-cds commented Jul 15, 2020

What about the label management stuff?

Label management would have to be adapted. It currently operates on a text set on the ElkLabel. To determine how large the label would be with the modified text, it uses this code:

if (rootRendering instanceof KText) {
// This is the easy case that we can handle quickly
newSize = PlacementUtil.estimateTextSize(((KText) rootRendering), text);
} else {
// Employ the big guns! Find the KText hidden in the rendering hierarchy, temporarily
// set its text override, estimate the size of the whole rendering, and kill the
// override again
KText kText = LabelManagementUtil.ktextFor(elkLabel);
kText.setProperty(KlighdOptions.LABELS_TEXT_OVERRIDE, text);
newSize = PlacementUtil.estimateSize(rootRendering, new Bounds(0, 0));
kText.setProperty(KlighdOptions.LABELS_TEXT_OVERRIDE, null);

This would have to be modified, but the problem is that it might not be clear how the modified text is to be distributed over the KTexts. Sounds almost as if ElkLabel needs an array of texts, which, I'm afraid, is not going to happen.

@sailingKieler
Copy link
Member Author

This would have to be modified, but the problem is that it might not be clear how the modified text is to be distributed over the KTexts. Sounds almost as if ElkLabel needs an array of texts, which, I'm afraid, is not going to happen.

I wouldn't do any sophisticated things here.
IMO we should keep the existing behavior as far as possible, KLighD should just not throw an exception if more than one KText ist found.

Are those label management components active by default?

@le-cds
Copy link
Member

le-cds commented Jul 15, 2020

Are those label management components active by default?

No, label managers have to be explicitly set on a graph through a property, so probably no harm done. Still, size estimation would have to be adapted for label management to be able to reserve enough space for all KTexts.

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
sailingKieler added a commit that referenced this issue Aug 3, 2020
klighd +.piccolo: enabled support of multiple KTexts per KLabel as demanded in #43
@NiklasRentzCAU
Copy link
Member

I just wanted to test this feature to close #13 and this issue as well, but can you please tell me how a client would override this guarding property MULTIPLE_KTEXTS_PER_KLABEL? I did not find a way to change it to true programmatically or from within some .kgt file, I only found setting the property hardcoded to a default true for testing this now.

For reference, this is the .kgt I tested this with:

kgraph Multitextlabel

knode a {
	size: width=10 height=10
	krectangle
	
	kedge (->b) {
		klabel {
			krectangle {
				styles: invisible=true
				grid: columns=2
				ktext("Text") {
					styles: foreground=127r
				}
				ktext("Text2") {
					styles: foreground=127b
				}
			}
		}
	}
}

knode b {
	size: width=10 height=10
	krectangle
}

I also needed to fix some NPE first, see other PR I will link here in a second.

@sailingKieler
Copy link
Member Author

Hi, if I'm right, you cannot activate it in the kgraph instance itself, but in the viewContext.
So you either set the property within the diagram synthesis on the viewContext - I'm not sure whether that's not too late -,
or, if you have a dedicated editor/view part, you can pre-configure the property e.g. in

@NiklasRentzCAU
Copy link
Member

I see, so this is a feature that has to be activated programatically. I tested setting it in the diagram synthesis - this is in fact too late - and in the DiagramViewPart, which worked. So as far as I understand overriding that would be necessary for a client to use the feature.

Do you want to add anything else for this issue (e.g. you mentioned individual selectability in the ticket description)? Otherwise I would close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants