Hi Vincent,

First of all, many thanks for your comments/feedback.

Vincent Hennebert wrote:
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.

My thinking behind initially switching this option off was to not break any existing configurations, but you are right its probably best to help the newbies and not worry too much about initially upsetting some of the power users. I take your point and will make this change.

- 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.

I will revert this change.

  - 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.

I'm not sure this is the case (but maybe it should be..). All the config fonts are loaded on demand by the LazyFont encapsulation. If you look at org.apache.fop.fonts.LazyFont.load(), the metrics url does take precedence over the embed url (see conditional tests on metricsFileName and fontEmbedPath).

    private void load(boolean fail) {
        if (!isMetricsLoaded) {
            try {
                if (metricsFileName != null) {
                    /[EMAIL PROTECTED] Possible thread problem here */
                    FontReader reader = null;
                    if (resolver != null) {
                        Source source = resolver.resolve(metricsFileName);
                        if (source == null) {
String err = "Cannot load font: failed to create Source from metrics file "
                                + metricsFileName;
                            if (fail) {
                                throw new RuntimeException(err);
                            } else {
                                log.error(err);
                            }
                            return;
                        }
                        InputStream in = null;
                        if (source instanceof StreamSource) {
                            in = ((StreamSource) source).getInputStream();
                        }
                        if (in == null && source.getSystemId() != null) {
in = new java.net.URL(source.getSystemId()).openStream();
                        }
                        if (in == null) {
String err = "Cannot load font: failed to create InputStream from" + " Source for metrics file " + metricsFileName;
                            if (fail) {
                                throw new RuntimeException(err);
                            } else {
                                log.error(err);
                            }
                            return;
                        }
                        InputSource src = new InputSource(in);
                        src.setSystemId(source.getSystemId());
                        reader = new FontReader(src);
                    } else {
                        reader
= new FontReader(new InputSource(new URL(metricsFileName).openStream()));
                    }
                    reader.setKerningEnabled(useKerning);
                    reader.setFontEmbedPath(fontEmbedPath);
                    reader.setResolver(resolver);
                    realFont = reader.getFont();
                } else {
                    if (fontEmbedPath == null) {
throw new RuntimeException("Cannot load font. No font URIs available.");
                    }
                    log.info("loadFont " + fontEmbedPath);
realFont = FontLoader.loadFont(fontEmbedPath, resolver);
                }
                if (realFont instanceof FontDescriptor) {
                    realFontDescriptor = (FontDescriptor) realFont;
                }
            } catch (FOPException fopex) {
log.error("Failed to read font metrics file " + metricsFileName, fopex);
                if (fail) {
                    throw new RuntimeException(fopex.getMessage());
                }
            } catch (IOException ioex) {
log.error("Failed to read font metrics file " + metricsFileName, ioex);
                if (fail) {
                    throw new RuntimeException(ioex.getMessage());
                }
            }
            isMetricsLoaded = true;
        }
    }

I would be interested to hear your comments on this.

  - 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.

I will change this.

- In apps.FopFactory:
  - AFAIU the validatesStrictly() method is not related to user
    configuration, as the javadoc seems to imply

This comment was added to the validatesStrictly method when Simon Pepping commented that these configuration options should be separate and I neglected to remove it. I will now remove this additional method comment.

  - setUserConfig doesn't throw SAXException or IOException, but
    FOPException

I am at pains to modify public APIs so as to remain backwards compatible and not break anything... There are three public setUserConfig methods, two of which throws SAXException, IOException so I tried to leave these as is - as a FopException is a subclass of SAXException anyways :). The other method needed to be changed though to report an exception (FopException) - instead of just logging it.

Cheers,

Adrian.

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.



Reply via email to