Hi Adrian,
Sorry for the delay. I've had a look at your patch. First of all, thanks
for taking care of this. Strong validation of user config files is
a great step towards user-friendliness. Thanks also for thinking of
documentation and testcases!
I have a few comments, some of which are open for discussion with other
devs:
- we may discuss about the interest of a separate/yet another parameter
for validating the config file. I can understand both points of vue
and, actually, I don't really care of which is chosen. That said,
I think validation of the config file should be enabled by default (as
strict FO validation, IIC) because this will be helpful to newbies.
Power users will be clever enough to disable it if they want to. So
I suggest you to change the default value to true in your patch.
- the changes in src/documentation/content/xdocs/0.93/fonts.xml are
a bit inexact. That's ok, you're probably still learning. But:
- the examples you added have different noticeable behaviours: the
first one gets the font's metrics through a beforehand generated XML
file and won't embed the font in the PDF file, because it doesn't
have access to the font file; the second ones computes the metrics
on the fly and is able to embed the font in the PDF file. As those
differences are not trivial and are explained further on the page
I would leave the example as is.
- if both files are specified the metrics-url won't take the
precedence, at least not exactly. The metrics defined in this file
will override the metrics in the font (if the user modified the file
by hand after generating it), but the embed-url file will always be
used to load and embed the font.
- AFAIK, kerning is available for PS and PDF outputs.
- but as I said, thanks for updating the documentation!
- I ask for other devs' opinions, but in strict user-config validation
mode I would not also log an error when an exception is subsequently
thrown. Reporting the problem just once is enough IMO.
- In apps.FopFactory:
- AFAIU the validatesStrictly() method is not related to user
configuration, as the javadoc seems to imply
- setUserConfig doesn't throw SAXException or IOException, but
FOPException
If no other fop-dev have objections to my comments above, I'd like to
ask you to post a new patch with the suggested corrections. It should be
ready to be applied then.
Thanks,
Vincent
> ------- Additional Comments From [EMAIL PROTECTED] 2007-02-02 03:55 -------
> Created an attachment (id=19497)
> --> (http://issues.apache.org/bugzilla/attachment.cgi?id=19497&action=view)
> main patch file
>
> (In reply to comment #3)
>> The patch is fairly large and seems to address more than only validation of
> the
>> configuration file. Can you give a summary?
>
> The patch seems quite large because I have added a new suite of configuration
> unit tests for the fix (these are contained in the attached zip file).
> A
> summary of the changes is given below.
>
>> I am against this exception being thrown if I do not use the font arial at
> all:
>> [ERROR] FOP - Exception
>> <org.apache.avalon.framework.configuration.ConfigurationException: Failed to
>> resolve font metric-url
>> 'arial.xml'>org.apache.avalon.framework.configuration.ConfigurationException:
>
>> Failed to resolve font metric-url 'arial.xml'
>
> In order for this to be the case, the user configuration would have to be
> (re-)read/parsed at the runtime of the rendering operation in order that it
> would be able to know if a particular font is being used in the given FO
> file.
> This would be an inefficient way of doing things and would require some
> structural changes.
>
>> I am also against combining strict validation of the FO file and the user
>> configuration; they are two different things. I would appreciate a separate
>> option 'validate-configuration', which checks every entry in the
> configuration file.
>
> I did raise this as a question on forum and I remember you responded to it.
>
> http://www.nabble.com/Bad-FOP-configurations-tf3064300.html#a8522251
>
> Chris Bowditch suggested that it be combined with "strict-validation".
> I can
> see your argument for this and this was my original idea but I can also see
> Chris' argument too. On balance I have decided to go with your suggestion and
> make it separate a separate configuration option so as to not cause any
> confusion.
>
> I have introduced a new configuration variable called "strict-configuration".
> I haven't called it "validate-configuration" because validation should always
> occur, its how the resulting error is handled that is the important thing (see
> Andreas' comments from the thread).
>
> If "strict-configuration" == "false" then FOP will simply log the error and
> continue to parse the user configuration. If "strict-configuration" == "true"
> then FOP will immediately raise an exception and will not continue trying to
> configure itself.
>
> I have attached a new patch file containing these changes (this includes
> related documentation changes). This new patch file superceeds the previous
> one.
>
> Kind regards,
>
> Adrian Cumiskey.
>
