[454629] Implement DotRecordLabel subgrammar.

[prggz]

• Create DotRecordLabel.xtext subgrammar
• Include generated Stub classes
• Implement DotRecordLabelTerminalConverters.java for UNQUOTED string
  terminal rule
• Implement DotRecordLabelJavaValidator.java methods in generated stub
  class
• Implement DotRecordLabelTests.xtend test class and include in
  AllUiTests.java
• Implement DotRecordLabel validation test case in
  DotValidatorTests.java

Signed-off-by: Zoey Gerrit Prigge zoey.prigge@uni-duesseldorf.de
Bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=454629

[eclipsewebmaster]

Issue tracker reference:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=454629

[miklossy]

I took a review and looks good.
@nyssen @mwienand could you please also take a look and merge?

[nyssen]

Thanks for the contribution. It seems the build fails because the respective record-related packages are not exported with x-friends to the tests bundle. Could you please adjust that, so Travis CI is able to successfully build again? I will then look into the details.

[prggz]

@nyssen Thanks for looking into this. I added another commit to address the issue. Should I squash them? I wasn't sure, if its safe to do this without creating a new branch/pull request. Or is it fine as is?

[nyssen]

Yes, you can squash both commits.

[prggz]

Okay, this should now be updated and the commit squashed with correct author info.

[nyssen]

Sorry for the late response, but we have now taken a deeper look into the contribution. It looks good so far, but we have identified two things we would like to see improved. First, it would be nice, if the original Graphviz grammar rule names (i.e. rlabel, field, fieldid) would be reused in the Xtext grammar. Further, we would propose to introduce a String data type rule (corresponding to the string rule in the Graphviz grammar), and to replace the UNQUOTEDSTRING terminal rule, so that only quoted whitespace can be part of the String. That would bring the grammar closer to the original Graphviz grammar, and it would make the terminal converter obsolete. Do you agree? Would you like to update the contribution accordingly?

[prggz]

Hi,

I felt the names were confusing in part and that's why I changed them. I will look into this again, however. I see how it might be more understandable to stick with not necessarily ideal names than to make up new ones (which are opinion based).

The UNQUOTEDSTRING rule essentially is the String rule in the original grammar. I have done it this way because if you look at Graphviz's rendering of PDF files it interpretes whitespace in HTML fashion.
I am aware that this is not identical to the specification which says that whitespace is a separator and will be ignored when drawing the graph. This does, imho not mean though, it's not allowed in the string, but that it will be ignored. Of course, I could allow a String datatype a list of whitespace separated strings - maybe that's what you meant? - when you say quoted whitespace you mean escaped whitespace (as there is no quoting ("...") within the label)?

I felt it is best, to simple ignore this discrepancy and align ourselves at Graphviz pdf output and dot file creation practice. What do you think considering these points?

On this note, I have in the meantime even considered to let the value converter remove double whitespace, line breaks and also resolve HTML escapes and the {, }, < and >, but not \n, \l, \r which are used later for formatting.

[mwienand]

Hi, cool stuff =)

I felt the names were confusing in part and that's why I changed them. I will look into this again, however. I see how it might be more understandable to stick with not necessarily ideal names than to make up new ones (which are opinion based).

Although the structure and the names chosen by you for the grammar rules are reasonable (I also prefer RotationWrapper to an inline rule), I would opt for sticking to the names used in the Graphviz documentation, because it helps to match Graphviz concepts to the implementation. Since we do not maintain documentation for the supported set of Graphviz Dot, users should be able to utilise the original Graphviz documentation.

Of course, I could allow a String datatype a list of whitespace separated strings - maybe that's what you meant? - when you say quoted whitespace you mean escaped whitespace (as there is no quoting ("...") within the label)?

Yes, I think using a String datatype that is comprised of multiple whitespace separated strings is a better alternative.

Yes, you are right, escaped whitespace.

I agree to this suggestion 100%, and I think it will be inline with the behaviour of Graphviz. With the current implementation, the record field name "Mid         Dle       \n       T e x t" will not get its duplicate whitespace removed. Moreover, the record field name "abc\ \ \ \ " would have its last space removed due to the value converter, which is incorrect IMHO (should increase label width which affects formatting/layout). OTOH, if the rules are changed, so that only escaped whitespace is considered for a String, the resulting record field name would be "Mid Dle \n T e x t" or "abc\ \ \ \ ", respectively. Although this does only make a difference for the first case when the String is not yet interpreted, I still consider it to be closer to the Graphviz documentation.

I felt it is best, to simple ignore this discrepancy and align ourselves at Graphviz pdf output and dot file creation practice. What do you think considering these points?

Yes, we should try to behave just like Graphviz, even though that may differ from the Graphviz documentation. However, I do not see how changing the terminal and introducing a data type rule would edge away from the Graphviz behaviour. Can you give an example?

On this note, I have in the meantime even considered to let the value converter remove double whitespace, line breaks and also resolve HTML escapes and the {, }, < and >, but not \n, \l, \r which are used later for formatting.

I would like to do without the value converter if possible.

[prggz]

I also prefer RotationWrapper to an inline rule

I think this might have had a reason in the created Type hierarchy. But I am not fully sure anymore. I would try to get this as close to the original grammar and add a comment if not and you can have a second look?

the record field name "abc\ \ \ \ " would have its last space removed due to the value converter, which is incorrect IMHO

It most certainly is, I agree.

the resulting record field name would be "Mid Dle \n T e x t" or "abc\ \ \ \ "

To be honest, this is the first time I wrote a grammar. So I thought, because Whitespace as such is hidden to the parser at the moment, it would become "MidDle\nText" and "abc\ \ \ \ ". I will have a look soon (sorry I am ill atm unfortunately) and check this. We will also need to check the behavior for abc\ \ .

Yes, we should try to behave just like Graphviz, even though that may differ from the Graphviz documentation. However, I do not see how changing the terminal and introducing a data type rule would edge away from the Graphviz behaviour. Can you give an example?

The way I understood the graphviz grammar, no unescaped whitespace can be part of a string.

Spaces are interpreted as separators between tokens, so they must be escaped if you want spaces in the text.

At first I thought you wanted me to make "Mid Dle" illegal input that does not parse, which of course is completely contrary to Graphviz behavior. However, I feel, this was a misunderstanding on my part.

I would like to do without the value converter if possible.

Thanks for the feedback. :)

[prggz]

@nyssen @mwienand
Hi, I have taken a look at the code now and changed the naming as wished.

I produced two versions

1. leave the rotation wrapper as is retaining the base class (Field) inherited by both FieldID and RotationWrapper
   [https://github.com/prggz/gef/commits/454629_RecordLabel_Grammar_v2]
2. have Field type with two attributes - which in DOT-language need not be in place at the same time - but there would not be any way of enforcing this in the model, if someone created fields manually. This would then introduce complication is later code. Further, if we want to iterate over the tree, for example, we can't test using instanceof but need to check for null in one of the attributes. (Given, we then would not need to cast).
   [https://github.com/prggz/gef/commits/454629_RecordLabel_Grammar_v2a]

Links to the relevant repos are above. Please have a look and I will then change this pull request or create a new one as you prefer.

Two notes on changing to the String datatype and eliminating the Port type - both in my pov are neglible or discussable but I wanted to quickly point them out, too.

1. We have hidden the type of whitespace. Unescaped newlines or tabstopps without any surrounding spaces (' ') would be ignored completely by graphviz. These are now also replaced by a single whitespace in our implementation. I am unsure, if this is significantly relevant, though; as I, too, would now prefer the version without the value converter. Luckily, in regards to \ Graphviz, too, does seem to handle this at a later point. I.e. \ followed by a tabstop is not rendered.

2. EDIT: have found a solution (warning against empty portname)

[prggz]

Can you also already have a look at this commit
[https://github.com/prggz/gef/commit/13c472137e33c19a11a49dd205324458e6202a2e]
If you are happy to, it might be ready to be included in the commit that will be merged. (I have needed to make these generalisations for the code concerning validation, checking against whether a record shape attribute is set in the nodes context).

[prggz]

@mwienand @nyssen
Have you had a chance to look at the changes I made? - see my comment from 02/10.

[nyssen]

Commit prggz@13c4721 looks reasonable. I think it makes sense to merge it into this contribution. Please prepare this contribution as it should be from your point of view. Linking to several alternatives is really hard to review (as its technically hard to compare). My wish would be that the solution is as close to the Graphviz grammar as can be (while the actual Graphviz behavior should have precedence over the documented behavior), and that it comes without the value converter.

[prggz]

@nyssen
Sorry for linking alternatives, I have now changed this branch and hope it is easier for you to review.

I have removed the value converter and the grammar is now (almost) identical in naming and structure to the graphviz one.

Hoping this satisfies your expectations.

[nyssen]

Thanks for the contribution, the final state looks good. Tamas, thanks for having reviewed and merged the changes.