kieler / KLighD

KIELER Lightweight Diagams
Eclipse Public License 2.0
34 stars 7 forks source link

Support of KLabels consisting of multiple KText fields #43

Open sailingKieler opened 4 years ago

sailingKieler commented 4 years ago

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 commented 4 years ago

@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?

NiklasRentzCAU commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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:

https://github.com/kieler/KLighD/blob/d21bf90432c4fecb9cf2899c93c64e00da86dc3a/plugins/de.cau.cs.kieler.klighd/src/de/cau/cs/kieler/klighd/labels/management/AbstractKlighdLabelManager.java#L253-L265

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 commented 4 years ago

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 commented 4 years ago

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.

NiklasRentzCAU commented 3 years ago

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 commented 3 years ago

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 https://github.com/kieler/KLighD/blob/de61aa1dd5a5507f97f572e7fa794ea46483fd26/plugins/de.cau.cs.kieler.klighd.ui/src/de/cau/cs/kieler/klighd/ui/parts/DiagramEditorPart.java#L635

NiklasRentzCAU commented 3 years ago

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.