Re: [xmlroff] 'border-style' and 'border-width' properties

[Tony G. <Ton...@Su...>]

Julian Rosse <gn...@th...> writes:
>   It looks like I've got border-style and border-width working, at least judging
> by a couple tests.

Can you send me your test files, too, so I can add them to the xmlroff
test suite?

>> The easiest way would be to submit the patch for
>> spec-dump/dump-info.xml and a separate patch for xmlroff/fo-context.c
>> and xmlroff/fo-context{-private}?.h.
>> 
>> Since the properties' files are new, I'll just merge your
>> dump-info.xml patch, make the properties' files, and copy them over.
>
>   I had to manually edit the new properties' .c files to change the validate
> function to check if it's a tblr (rather than the default set of validation
> type checks) - I copied this from fo-property-border-color.c.  So I should
> send you at least the .c files, unless you want to make this change to each
> by hand.

Patches would be good.

>   In terms of sending patches, what is the easiest/best way to do that?  Can
> you create a patch that applies to multiple files?  Just do "cvs diff file1
> file2 ..."?  Above you seem to be implying a certain way of splitting up the

If the only changes you've made to the source are for 'border-width'
and 'border-style', then just 'cvs diff' should be sufficient.

> patches.  I also edited property/fo-property-eval.c and
> property/fo-all-property.h (patching in relevant lines from spec-dump's
> "make fo-all-property-h-dump") in addition to the above-mentioned dump-info.xml,
> fo-context-*, the below-mentioned property/Makefile.am, and the new
> properties' .c files (their .h files as dumped by spec-dump are ok, I guess - 
> I didn't touch them).  And then the property resolution code in
> fo_context_util_border_resolve() in fo-context-util.c.
> I think that's it.  So I guess I'm asking: for those
> files, what's the best way to submit patches to you?

One patch file for the 'xmlroff' module and one for the 'spec-dump'
module.

You can send them straight to me.

>> I have modified 'dump-info.xml' to reduce the differences.
>> 
>> I also added support for a 'expr-eval' attribute on <property>
>> elements in dump-info.xml so there'll be one less piece of manual
>> fixup on the generated files.
>
>   Cool; I used the expr-eval attribute, using your new
> fo_expr_border_style_eval() for border-style and fo_expr_padding_eval() as
> you recommended for border-width.  I also set the resolve-enum attributes
> to the same thing that the individual border-style/border-width properties
> have for that attribute - I was sort of copying what you did for border-color
> and am not sure whether these resolve-enum attributes are entirely correct/
> necessary.

The validation code for FoTblr property values really should call a
validation routine for each of its component values, but obviously it
doesn't yet.

The functions named in the resolve-enum should be called at some point
as part of validating the FoTblr property values because they do
things like convert the 'medium' border-width enumeration token into a
length.  The whole resolve-enum and validate functions machinery was
set up for the non-shorthand properties and hasn't caught up with the
fact that we also do shorthands.

The resolve-enum functions are also part of the context for evaluating
a property value expression.  Evaluating the 'border' property will
probably require a resolve-enum function that just calls the three
resolve-enums for the three components to see if any of them can
resolve the enumeration token.

Suggestions on how to improve it would be welcome.

>   Another thing - in my test cases, when I used "border-width", it gave me
> a number of warning messages equal to the number of tokens in border-width's
> value about datatype != NULL failing in fo_expr_context_push_stack().  Looking
> at eval_border_style_expr(), I'm pretty sure you just have an extra
>
> fo_expr_context_push_stack (context, intermediate_value);

I'll look at it once I have running code.  Thanks for pointing it out.

> where you don't want it - it looks like eval_enum() actually does the
> pushing earlier in the function and intermediate_value doesn't get set if
> parse_ncname() succeeds, thus the error indicating that intermediate_value
> is NULL.  It was working nonetheless, I guess because eval_enum() was
> successfully pushing the tokens onto the stack, but letting you know.

Thanks for doing this work.

Regards,


Tony.