| Re: [elk-dev] Meta Data Language |
Hi everyone, I finally got around to looking at the new language. Nice work, Miro! Here are my general major observations: - I don't think I really understand why we need the 'duplicated' keyword. As far as I understand it, adding that keyword adds our own Property constant for a layout option in the generated Properties class. This mostly seems to be relevant for when we change the default value of a property. How about this instead: We always generate our own properties. Thus, when we change default values, we don't need to change our algorithm's code to new property declarations since it already uses our own declarations anyway. If we declare an explicit default value, the generated code includes default value constants and uses those to initialize the generated properties. If we don't specify explicit values, the default value declared in the original Property is used. I think this would make it a bit easier to understand how everything works. - I understand what it means for a layout option to be advanced, programmatic, or output option. What does it mean to be a global option? Minor Remarks - Generated Properties classes: - Don't adhere to four spaces for indentation (if there are formatter preferences for the meta data language, we should set them properly in the Oomph setup) - Fields should be "public static final" instead of "public final static" - File header comment does not adhere to standards - .elkm files - Contain tabs instead of spaces for indentation (again, Oomph setup if there are formatter preferences) Replies to the issues you raised are inlined below: On 01/02/16 10:39, Miro Spönemann wrote: > The generated code is currently not checked in, so you have to > run GenerateMetaData.mwe2 manually. We should decide whether to check in > the code or not and then apply the same to the Graphviz Dot language. > It’s not hard to generate the code during the auto-build: > > https://www.eclipse.org/Xtext/documentation/350_continuous_integration.html I'd love the generated code to be checked in. This way, the repository state works out of the box after checkout. While we could probably run MWE2 workflows automatically during Oomph (haven't checked that one, but I bet it's possible), this would be required each time someone pushes changes that require code to be regenerated. > Furthermore, you will have to start a new Eclipse instance in order to > use the meta data text editor. As soon as we have included the new > plugins into the build, we can add them to the update site and include > them in the Oomph setup so our developer instance will already have that > editor. I've opened a ticket for changing the Oomph setup accordingly: https://github.com/eclipse/elk/issues/7 > - Some enum options have an ‘UNDEFINED’ value > (Direction, EdgeLabelPlacement, EdgeRouting, PortAlignment, PortConstraints, PortSide). > We could remove those and set the default to null for the general > option. Then we should make sure every algorithm that supports these > options defines proper default values for them and uses the ‘duplicated’ > keyword in its ‘supports’ declaration in order to generate a Property > with overridden default value (see e.g. how this works for SPACING). > Advantage: UNDEFINED would not be visible in the Layout view anymore. +1 for removing UNDEFINED. > - The tree algorithm has plugin id org.eclipse.elk.alg.mrtree but > package name org.eclipse.elk.alg.tree I'll change that: https://github.com/eclipse/elk/issues/8 > - How should we proceed with lower / upper bounds? For now I removed > those from the meta data. I am thinking about introducing a more general > approach of “graph validation”, which could be applied before any > algorithm is executed. My feeling is that it would be more useful to > throw an exception in this pre-layout phase instead of silently ignoring > negative spacing values. If the source of the wrong values is > programmatic, they should be fixed anyway. If the source is the UI > (Layout view), ideally we should mark the wrong values there so the user > gets an early feedback about nonsense configurations. My proposal would be two-fold. One, declare min/max bounds, if necessary, in the meta data language. This information can be used for the UI hints you mentioned. Second, have the meta language code generate a validateProperties(IPropertyHolder) method that can be called from the layout algorithm to throw an exception if bounds are invalid. > - Now we’re ready to generate documentation for algorithms and layout > options. What would be a suitable format for that? And is there > additional information would we like to add into the meta data in order > to enhance the documentation? I think the target format should be Markdown, to be used in the ELK Wiki. The description of a property could be the synopsis, while the actual documentation would be longer and may include references to images. Since the documentation can be quite long, it may be best to place it in markdown files somewhere inside a folder in the plug-in that declares a layout option, along with all required image files. The folder those files can be found in could be specified as a bundle property, while the exact file name could become a property... well, property. What exactly the generated documentation will look like can be the subject of a subsequent discussion.
Attachment:
signature.asc
Description: OpenPGP digital signature