Specify MSC attributes similar to arc attributes

[GoogleCodeExporter]

This issue requests an enhancement to the way attributes/options are specified 
for msc. Currently it is specified immediately following "msc {" as in "msc { 
opt1=value,opt2=value". Change required is to specify through "msc { 
[attr=value, ...]", similar to arc attributes.

This is just a preference which I believe has some advantages.

(1) Parser can give meaningful error message if the attribute is mistyped, 
since parser knows that it is an attribute which is mistyped and will not take 
it as an entity name.

(2) Old version of mscgen can possibly ignore any unknown attribute after 
giving a warning message and continue generating msc.

(3) Visual clue in msc language that they are options.

Without '[]', the 3 options currently available in mscgen shall continue to 
work for backward compatibility.

Original issue reported on code.google.com by v.arunmo...@gmail.com on 17 Sep 2010 at 7:40

[GoogleCodeExporter]

Original comment by NThykier@gmail.com on 17 Sep 2010 at 11:15

• Added labels: Component-Persistence, OpSys-All, Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

Unfortunately adding square brackets here won't improve the error messages 
generated.  The problem is that yyerror() doesn't get much context to report 
and adding square brackets won't change that.  Additionally the attribute names 
are tokenised by the scanner so rejected very early on.

Changing the grammar also risks backwards-incompatibility and could result in 
more confusion as to the correct syntax.

Also note that r145 has already improved the error message handling by 
displaying the faulty input line.

I'm going to close this as wontfix, as I'm not sure there's enough of a problem 
or solution here.  Note that we can reopen if the issue gains more weight.

Original comment by Michael....@gmail.com on 6 Oct 2010 at 8:34

• Changed state: WontFix

[GoogleCodeExporter]

I think the idea behind using [] is to do something like:

opts: '[' opt_list ']' | /* empty */ ;

opt_list: opt | opt_list ',' opt;

opt: IDENTIFIER '=' STRING

and then the opt rule could check if the IDENTIFIER matched a known attribute.

Alternatively we can add an additional clause to the current opt to allow 
"IDENTIFIER '=' STRING" and if that rule is matched, print an error saying that 
the attribute is not known and then abort parsing.

Though admittedly I have not seen what mscgen prints in this case after r145, 
so I cannot tell whether it is needed.

Original comment by NThykier@gmail.com on 7 Oct 2010 at 1:05

[GoogleCodeExporter]

The current grammar already validates options through the following rules:

opt:         optval TOK_EQUAL string;

optval:      TOK_OPT_HSCALE | TOK_OPT_WIDTH | TOK_OPT_ARCGRADIENT;

The problem of unknown options not providing meaningful error messages is 
really rooted at the following: 

msc: TOK_MSC TOK_OCBRACKET optlist TOK_SEMICOLON entitylist TOK_SEMICOLON 
arclist TOK_SEMICOLON TOK_CCBRACKET
     | TOK_MSC TOK_OCBRACKET entitylist TOK_SEMICOLON arclist TOK_SEMICOLON TOK_CCBRACKET;

If the first token isn't a valid 'optval', the second form of the 'msc' rule 
without the 'optlist' is used as that will accept a 'string'.  Because of this, 
parsing will continue a little further and then the eventual error message 
isn't great because the parser is off looking for entity declarations.

If the grammar were changed to add some square brackets around the 'optlist', 
we could get a more meaningful message as the parser would end up needing an 
'optval' and couldn't continue.  However, making the square brackets optional 
would undo this and again go back to the current situation.  

Not wanting to break backward compatibility, I don't see a good enhancement to 
be had here, noting that there's actually only 3 attributes defined.  Some of 
the example files also show attributes defined in the files.

> Though admittedly I have not seen what mscgen prints in this case after r145, 
so I cannot tell whether it is needed.

Here's an example:

echo 'msc {
   bad=5;
   A, B, C, "D";
   "A" -> "B" [label="sigab", url="bob"];
}' | mscgen -Tpng -o /dev/null

Error detected at line 2: syntax error, unexpected '=', expecting ',' or ';'.
>    bad=5;

I don't think this is so terrible.

Original comment by Michael....@gmail.com on 7 Oct 2010 at 9:29

[GoogleCodeExporter]

Coming back to the context why this enhancement was initially requested: I 
wanted to unify the way attribute was specified regardless of whether it is for 
msc, entity or arc. The reason for this is that the attributes can later be 
overridden (e.g. a particular arc can have arcgradient/color changed from what 
was specified initially).

This kind of overriding can be within msc to create a kind of scope as in 
example below

msc {

    [ arcgradient=10, linecolor=black ]; # initial/default attributes

    A, B, C;

    A -> B;
    B -> C;
    C -> B [ arcgradient=5 ]; # Affects only this arc

    [ arcgradient=0 ]; # Affects all the following arcs

    A -> B;
    B -> C;

    # May be even possible to create a scope like this
    {
        [linecolor=gray];
        A -> B;
        B -> C;
    }
}

The second point is that, more such msc attributes can be easily added for the 
future (e.g. specifying default attributes for arcs, attribute/command macros, 
spacing attributes etc).

The other points initially mentioned are just trivial points to minimize the 
enhancement requested.

Also it is expected that other than the three exisiting attributes, newly added 
attributes cannot be specified without []. This ensures that we are backward 
compatible.  We do not actually make [] optional. As  NThykier pointed out, 
attributes inside [] are parsed as IDENTIFIER token, and without [] the three 
exisiting attributes are parsed as individual tokens (TOK_OPT_HSCALE, 
TOK_OPT_WIDTH, TOK_OPT_ARCGRADIENT) as it is now.

Original comment by arunmozh...@gmail.com on 8 Oct 2010 at 5:02

[GoogleCodeExporter]

Will this issue be reopened? Can anyone reopen this issue.

Original comment by v.arunmo...@gmail.com on 12 Oct 2010 at 2:18

[GoogleCodeExporter]

If the request were to make arcgradient settable per-arc, I'd consider that as 
it has no change to the general grammar with which others are familiar.   
Similarly, while 'arclinecolour' and 'arctextcolour' can already be used to set 
default colours which can be overridden (notably based on entity though), I can 
see that a global arcline/textcolour might be useful for some as well.

A syntax change is orthogonal to new arc attributes or global settings.  So I'm 
still not keen on syntax changes as it may cause problems or confusion for 
others and the overall benefit is unclear.

If others comment on this issue and want a syntax change, maybe it will give 
weight to the argument and this can be reopened.

Original comment by Michael....@gmail.com on 12 Oct 2010 at 8:07

[GoogleCodeExporter]

That should be fine if msc attributes are available to be changed per arc. May 
be a separate enhancement request raised for this.

Original comment by v.arunmo...@gmail.com on 13 Oct 2010 at 2:21