One thing I forgot to mention: this work deserves its entry in the
status.xml file, with importance set to ‘high’.
On 04/07/12 08:38, mehdi houshmand wrote:
> On 3 July 2012 15:48, Vincent Hennebert wrote:
<snip/>
>> The new classes FopConfParser, FopFactoryBuilder and FopFactoryConfig
>> should be renamed into FOPConfParser, FOPFactoryBuilder and
>> FOPFactoryConfig. The fact that some existing public classes have Fop in
>> lower case in their names was a mistake that was discovered after the
>> API was frozen. Let’s not do that mistake again.
>>
>
> I don't agree, I think this makes the classes more inconsistent rather than
> less so. I don't particularly have an opinion whether they should be Fop*
> or FOP* but I think either way, there should be consistency
Let me insist on that one. The inconsistency is actually introduced by
Fop classes. Before the merge there were 14 classes having FOP in their
names, vs 6 having Fop. Also, the product name is FOP, not Fop, and that
should be reflected in the class names.
<snip/>
> • getRendererConfig(String mimeType, RendererConfigParser configCreator)
>> isn’t there a risk of mismatch between the given mimeType and
>> configCreator? Can’t the mimeType be taken from the
>> RendererConfigParser.getMimeType method?
>>
>
> No. The same parser can be used for more than one MIME type and we don't
> want the config data to be retrieved from the wrong MIME type.
I’m confused. It seems that I can call that method with an
‘application/pdf’ mimeType and a PSRendererConfigParser instance. In
which case the parser would not be able to parse PDF-specific options,
and would silently ignore them. Or did I miss something?
<snip/>
>> • getHyphPatNames()
>> this is hard to pronounce; also ‘Pat’ can easily be mistaken for
>> ‘Path’
>> s/getHyphPatNames/getHyphenationPatternNames/?
>>
>
> Luckily it's written so you don't have to pronounce it ;). No seriously,
> done.
Thanks, my brain’s internal read aloud feature was stumbling upon that
one :-)
>> src/java/org/apache/fop/apps/FopConfParser.java
>> • there’s a mistake in the handling of ‘strict-validation’: this is
>> related to checking the conformance of XSL-FO documents against the
>> spec. The setting to strictly check the validity of config files is
>> ‘strict-configuration’
>>
>
> Again, there's a TODO, I don't think this is a regression but just
> something that was confused previously, though again, time constraints...
There does seem to be a regression. Before, the FopFactoryConfigurator
object was getting the strict-validation element from the config file
and calling FopFactory.setStrictValidation if it was set to true. This
is no longer the case.
I’ve just tried to render a table with table-footer after table-body,
and now a validation error is thrown even if I have strict-validation
set to false in my config file. No validation error was thrown before.
>> src/java/org/apache/fop/apps/FopFactoryBuilder.java
>> • the buildConfig method is deprecated but is used by the build method
>> underneath, which is itself not deprecated. But then it shouldn’t rely
>> on a deprecated method?
>>
>
> The javadoc makes it clear that the only reason this is there is to
> maintain backwards compatibility, we'll remove it at some point in the
> future when users are more comfortable with the new API design.
So when you remove it you will have to re-write the build method such
that it doesn’t call buildConfig anymore. How will you do? (Admittedly
I haven’t studied the new API in much detail.)
And BTW, what is the recommended way to get a FopFactoryConfig? The
javadoc of buildConfig doesn’t say. I think it should.
>> src/java/org/apache/fop/apps/FopFactoryConfig.java
>> • This is an interface whose methods’ javadocs refer to the FopFactory
>> concrete class. So an abstraction depends on a concrete
>> implementation. Surely that should be the other way around?
>>
>
> Again, this is to make the transition easier for users. Users are used to
> accessing these members via the FopFactory and the javadocs help lubricate
> the transition.
I’m not sure I understand. How is that supposed to help? Wouldn’t it
actually be more helpful if the FopFactory were now referring to the
interface? That would indicate to users that something is going to
change and they should take the habit of looking at FopFactoryConfig
from now on.
>> src/java/org/apache/fop/apps/io/InternalResourceResolver
>> • what does ‘Internal’ stand for?
>>
>
> Stand for? It doesn't stand for anything. This class is FOPs internal
> resource resolver that does some primitive URI validation when strings are
> given and holds the base URI when relative URIs are given.
Why is it a public class then if it’s supposed to be internal to FOP?
IIC that apps/io package is meant to become part of the public API?
>> • getBaseURI: I don’t think adding a slash at the end of the URI if
>> it’s
>> not there is desirable, because that effectively changes the base URI.
>> That may lead to very confusing errors.
>>
>
> As the javadoc suggests this is for giving you a directory base URI, I've
> changed the method signature to {getBaseURI, getBaseDirectoryURI}.
Ah, I’m glad to hear that I was right to assume that the getBaseURI
method returns a base URI :-)
What I’m talking about is that, by adding a final ‘/’ to the given
string, you are effectively changing the base URI.
For example, that method is called by the AFPResourceAccessor class,
which passes as a parameter the baseURI it’s been given, which that
getBaseURI method may change into a different base URI! This is very
misleading and may result into hard-to-find errors.
>> src/java/org/apache/fop/apps/io/TempResourceResolver
>> • unless I’m mistaken there’s no way of releasing the temporary resource
>> once it’s no longer needed. The DefaultTempResourceResolver
>> implementation deleteOnExits the file but this may not be enough if
>> FOP is run on a server that is meant to (almost) never shut down. Or
>> did I miss something?
>>
>
> You missed something; this is the default implementation. This is pretty
> much only for the CLI use-case, if users wish to define their own
> implementation, all they have to do is implement the interface.
OK, so presuming I want to write my own implementation that does a more
fine-grained management of resources, how do I know that I can release
a given resource?
<snip/>
Vincent
